Repository navigation
Conversation
…the export extra forgelm export runs llama.cpp's convert_hf_to_gguf.py with ForgeLM's own Python. The installation page, the export extra's comment in pyproject.toml, the module docstring, the usage and quickstart guides and the post-training notebook said it was llama-cpp-python's "bundled" script, and the error hint advised `pip install 'llama-cpp-python>=0.2.90'`. No released wheel puts the script where ForgeLM searches (0.2.90 installs it under bin/, 0.3.x does not ship it), so a plain install ends with "convert_hf_to_gguf.py not found inside the llama-cpp-python installation" (exit 2) unless FORGELM_GGUF_CONVERTER points at the script in a llama.cpp checkout. The upstream script imports sibling modules (conversion, gguf), which is why the checkout's copy has to be used; the upstream repository and the requirements path were checked. The pages now say so, with the two commands that make it work; the error message names FORGELM_GGUF_CONVERTER and a llama.cpp checkout instead of a reinstall, and a test pins that. Searching the locations real wheels use, or bundling the script, are code changes and stay open. Refs #113, #83 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Sorry @cemililik, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 12 hours and 45 minutes by commenting |
Contributor
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 51 minutes. View limit details
Comment |
Reviewer's GuideThis documentation-focused PR corrects the GGUF export installation contract: Sequence diagram for GGUF export converter discoverysequenceDiagram
participant User
participant ForgeLM
participant LlamaCpp
participant Converter
User->>ForgeLM: forgelm export
ForgeLM->>ForgeLM: _find_converter_script()
alt FORGELM_GGUF_CONVERTER is set
ForgeLM->>LlamaCpp: Resolve convert_hf_to_gguf.py
LlamaCpp-->>ForgeLM: Converter path
ForgeLM->>Converter: Run with ForgeLM Python
Converter-->>ForgeLM: GGUF output
else Converter is not found in llama-cpp-python
ForgeLM-->>User: FileNotFoundError with checkout and env-var hint
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5 tasks done
Collaborator
Author
|
Superseded by #998: the wave-0 fixes are combined by theme into five pull requests to cut the review load. This branch's commits are included there unchanged apart from the |
cemililik
added a commit
that referenced
this pull request
Oct 18, 2026
…cument the converter requirement (#998) ## Summary `forgelm export` now writes the SHA-256 sidecar that `forgelm verify-gguf` and the documentation already assumed, and the GGUF documentation describes what really happens. Two earlier pull requests combined into one (#982 and #995, which will be closed as superseded); the bot findings on #982 are folded in. ## Related issues Refs #84, #85, #86 (docs), #83, #113. Making `verify-gguf` stricter than "magic header + optional sidecar", and searching the locations real llama-cpp-python wheels use for the converter, are separate changes and stay open. The site strings that mention the sidecar are in a later pull request. ## Changes ### Sidecar (was #982) - `forgelm/export.py`: after the GGUF is written and hashed, `<output>.sha256` is written in `sha256sum` format (`<digest> *<name>`) through a temporary file and `os.replace`. A write failure returns `success=False` (runtime error, exit 2) naming the GGUF path. For a K-quant request the file written is the intermediate `.f16.gguf` and the sidecar belongs to it. The `model_integrity.json` update failure is now a `WARNING`, not `DEBUG`. - The sidecar detects corruption and accidental change; it is **not** tamper evidence, because whoever can replace the GGUF can replace the sidecar next to it. The docs say so. - `docs/usermanuals/{en,tr}/deployment/{gguf-export,verify-gguf}.md` and `docs/reference/verify_gguf_subcommand{,-tr}.md`: real output, the K-quant two-step flow, the real `exported_artifacts` JSON, what `verify-gguf` checks and what it cannot, and the `--adapter` pitfall. Review findings applied: the intermediate file is named by replacing `.gguf` with `.f16.gguf`; the manifest entry is best effort. ### Converter script (was #995) - `pip install 'forgelm[export]'` does not provide `convert_hf_to_gguf.py`: no released llama-cpp-python wheel puts it where ForgeLM searches (0.2.90 installs it under `bin/`, 0.3.x does not ship it). The installation page, CLI reference, usage and quickstart guides, the notebook, `pyproject.toml` comment and `export.py` docstring now say to point `FORGELM_GGUF_CONVERTER` at the script in a llama.cpp checkout; the upstream repository location, requirements path and imports were checked. - The not-found error message names `FORGELM_GGUF_CONVERTER` and a llama.cpp checkout instead of advising a reinstall. ## Type - [x] Feature (sidecar) - [x] Documentation ## Testing - `tests/test_export.py`: sidecar format and content, `sha256sum -c` and `verify-gguf` accept it, atomic write and cleanup, K-quant sidecar, failure path, plus the new converter hint test (**fails against the old message**). - The full `CLAUDE.md` gauntlet from a clean worktree of this branch: exit 0. ## Checklist - [x] English and Turkish mirrors updated together (Turkish wording needs the owner's review) - [x] CHANGELOG entries under `[Unreleased]` - [x] Tests added 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by Sourcery Add reliable SHA-256 sidecars to GGUF export and align converter requirements, verification behavior, and user documentation with the implemented workflow. New Features: - Write SHA-256 sidecar files for exported GGUF artifacts, including intermediate files produced for K-quant requests. - Fail exports when the required sidecar cannot be written and elevate integrity-manifest update failures to warnings. Bug Fixes: - Correct GGUF export and verification documentation to reflect actual outputs, checks, limitations, K-quant workflow, and adapter usage. - Correct converter setup guidance and error messaging to require a llama.cpp checkout through FORGELM_GGUF_CONVERTER. Enhancements: - Clarify that sidecars detect accidental corruption but do not provide tamper evidence. - Document the actual exported_artifacts manifest structure and verification behavior in English and Turkish documentation. Documentation: - Update installation, CLI, usage, quickstart, notebook, and GGUF deployment documentation with the llama.cpp converter prerequisite and sidecar workflow. Tests: - Add coverage for sidecar format, verification, atomic cleanup, K-quant output, write failures, manifest warnings, and converter guidance. Chores: - Update the unreleased changelog and related export comments and docstrings. --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pip install 'forgelm[export]'was described as bringing llama-cpp-python's "bundled"convert_hf_to_gguf.py, and the error hint advisedpip install 'llama-cpp-python>=0.2.90'.forgelm exportruns llama.cpp's converter with ForgeLM's own Python, and no released llama-cpp-python wheel puts the script where ForgeLM looks (0.2.90 installs it underbin/, 0.3.x does not ship it). A plain install ends withconvert_hf_to_gguf.py not found inside the llama-cpp-python installation(exit 2) unlessFORGELM_GGUF_CONVERTERpoints at the script in a llama.cpp checkout.Related issues
Refs #113, #83 — the documentation half and the misleading error hint. Searching the locations real wheels use (or bundling the script) is a code change and stays open. The GGUF export and verify pages themselves are in the sidecar PR.
Changes
docs/usermanuals/{en,tr}/getting-started/installation.md: the[export]section now says the script is not included and gives the two commands that make it work (git clone https://github.com/ggml-org/llama.cpp;export FORGELM_GGUF_CONVERTER=…/convert_hf_to_gguf.py). It explains why the checkout's copy is needed: the upstream script imports sibling modules (conversion,gguf), and its requirements must be importable in the environment ForgeLM runs in. I checked the upstream repository location, therequirements/requirements-convert_hf_to_gguf.txtpath and those imports against upstreammaster.FORGELM_GGUF_CONVERTERrow, EN+TR), the usage and quickstart guides (EN+TR), the post-training notebook cell, theexportextra's comment inpyproject.tomland theexport.pymodule docstring say the same.forgelm/export.py: the not-found message now says "SetFORGELM_GGUF_CONVERTER=/path/to/llama.cpp/convert_hf_to_gguf.py, the script inside a llama.cpp checkout" instead of advising a reinstall. No other code changes.Trade-off
The documented route is a llama.cpp checkout plus an environment variable. That is the only route that works today; I did not add an env-free path.
Type
Testing
tests/test_export.py::TestExportModel::test_missing_converter_script_in_package_is_runtime_errornow also asserts the new hint (namesFORGELM_GGUF_CONVERTERand "llama.cpp checkout", nopip install); it fails against the old message.CLAUDE.mdgauntlet from a clean worktree of this branch: exit 0.Checklist
[Unreleased]🤖 Generated with Claude Code
Summary by Sourcery
Clarify that GGUF export requires the converter from a llama.cpp checkout and direct users to configure it explicitly.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: