Conversation
M5. `bloomery export` writes a GGUF and an Ollama Modelfile, at f16, q8_0 or q4_0. Written here rather than handed to llama.cpp's convert_hf_to_gguf.py. That converter identifies a tokenizer by hashing its output against a table of known models, and a tokenizer this project trained is a new hash by construction -- measured 74a7f913c36b5d2f on a real checkpoint. So the stock path refuses precisely the from-scratch models bloomery leads with. Refusing is right for a general tool, since the hash picks the pre-tokenizer regex and the wrong one yields a model that loads and talks nonsense. But we hold the answer it is trying to recover: these are byte-level BPE with the standard regex, so the field is set outright. The README claimed the opposite -- "GGUF conversion, vLLM and Ollama all work without a bespoke converter" -- since before any of this existed. It now says what is true. What the round trip proved, and what it did not Tensors, vocabulary, metadata and the tied output head all verified by reading the file back. Every one of those passed while the artefact was unloadable: BPE is its merge rules, not just its vocabulary, and llama.cpp refuses a file without them -- "cannot find tokenizer merges in model file". That was found by running the export through Ollama, not by any test here, and there is now a test for it. Verified end to end: `ollama create` accepts the file and the model generates, 16 tokens, done_reason length. Details worth recording Embeddings are tied, so the safetensors hold no lm_head. Loading materialises one, and it is derived from the embedding when it does not. Normalisation weights stay f32 and the embedding f16 even under q4_0, which is what llama.cpp's own quantizer does: a small share of the bytes and a large share of the behaviour. The K-quants are absent because the gguf package raises NotImplementedError when asked to write one. It can read them, which is a different thing. The command names llama-quantize rather than pretending. LoRA checkpoints are merged first. GGUF has no notion of an adapter, so without the merge the export is silently of the untouched base. context_length is the sequence length the model was trained at, not a capability, and a runtime holds it as a hard limit. It is reported. Also fixes a pre-existing flake The memory pre-flight derives its budget from *available* RAM, and a depth-1 model estimates ~0.8 GiB once fixed overhead is counted. On a constrained machine the budget dips under that partway through a suite run, and `train` or `adapt` refuses -- so a test about tokenizers fails for reasons of its host. It showed up as this file passing alone and failing inside the full suite, a different test each time. 32 of 34 invocations were exposed. The budget is now pinned for CLI tests, with TestMemoryGuard opting out because it is what tests the real one.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds GGUF export for Llama and Mistral checkpoints. Supports f16, q8_0, and q4_0 formats, LoRA merging, tokenizer metadata, Ollama Modelfiles, CLI and job integration, checkpoint-safe replacement, tests, CI setup, and documentation. ChangesGGUF export
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ModelLoader
participant AdapterMerge
participant GGUFWriter
participant Modelfile
CLI->>ModelLoader: load checkpoint and tokenizer
CLI->>AdapterMerge: merge LoRA adapters
AdapterMerge->>GGUFWriter: provide evaluation model
GGUFWriter->>GGUFWriter: write GGUF tensors and metadata
CLI->>Modelfile: generate adjacent Ollama Modelfile
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/bloomery/export.py (1)
243-245: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAn eos token containing
"or\produces a malformed Modelfile.Line 245 interpolates
eosinto a double-quoted Modelfile value with no escaping. A token such as<|end"of"text|>closes the string early and Ollama reads the rest as stray arguments. This is speculative for tokenizers bloomery trains — their eos tokens are angle-bracket forms — butexport --checkpointaccepts any checkpoint, so the input is not under this project's control.🔧 Proposed fix
eos = getattr(tokenizer, "eos_token", None) if eos: - lines.append(f'PARAMETER stop "{eos}"') + # Escaped: the value is quoted, and the token comes from whatever + # checkpoint was handed to `export --checkpoint`. + escaped = str(eos).replace("\\", "\\\\").replace('"', '\\"') + lines.append(f'PARAMETER stop "{escaped}"')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bloomery/export.py` around lines 243 - 245, Escape backslashes and double quotes in the eos value before interpolating it into the quoted Modelfile stop parameter. Update the export logic around eos and the lines.append call so arbitrary checkpoint tokenizer values produce a valid Modelfile while preserving the existing omission when no eos token is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 185-194: Narrow the ecosystem compatibility claim in the README
passage around “vLLM and anything else in the ecosystem” to “vLLM and other
runtimes that support Llama Hugging Face checkpoints.” Do not claim untested
broad ecosystem support, and preserve the existing explanation of GGUF handling.
In `@src/bloomery/cli.py`:
- Around line 1192-1198: Ensure all export failures remove the staging
directory: in src/bloomery/cli.py lines 1192-1198, use a success flag with
finally so cleanup runs on every exit path while preserving successful
completion behavior; in src/bloomery/export.py lines 141-143, update the
model.embed_tokens.weight lookup to raise ExportError when absent instead of
propagating KeyError.
- Around line 1200-1202: Update the export replacement flow around destination
and staging so it first renames the existing destination to a temporary backup,
then renames staging into destination, and only deletes the backup after the new
export is successfully installed. Preserve the existing staged-write behavior
and ensure a failed replacement does not remove the previous good export.
In `@src/bloomery/export.py`:
- Around line 141-143: Update the state handling in the export flow around the
lm_head.weight fallback so a missing model.embed_tokens.weight is converted into
the module’s established ExportError rather than propagating KeyError. Preserve
the existing fallback when the embedding weight exists, and use the same
refusal/error-message pattern as the surrounding export validation.
- Around line 145-150: Update the GGUFWriter construction in the export flow to
pass "llama" when config.model_type is "mistral", while preserving the original
model_type in ExportResult.architecture for reporting. Add a regression test
asserting the written metadata architecture value is "llama".
- Around line 319-325: Update the vocabulary construction before
writer.add_token_list in the export flow to index tokens by their tokenizer IDs
rather than sorting token strings. Build exactly config.vocab_size entries,
explicitly fill missing IDs, and reject any token IDs outside the valid range so
the list remains aligned with token_embd.weight.
In `@src/bloomery/jobs/types.py`:
- Around line 57-59: Update the EXCLUSIVE_KINDS definition to include
JobKind.EXPORT, ensuring export jobs cannot run concurrently with training or
checkpoint updates. Preserve the existing exclusive handling for TRAIN, ADAPT,
and BENCH.
In `@src/bloomery/server/static/app.js`:
- Around line 68-73: Update the `run` and `checkpoint` field definitions in the
`export` form so both hints explicitly state that the fields are mutually
exclusive and exactly one must be provided. Preserve the existing labels and
other field behavior; if the form already supports submit validation, also
reject submissions where both fields or neither field is populated before
queueing.
In `@tests/test_export.py`:
- Around line 184-196: Update test_a_chat_template_is_carried_across to preserve
the shared tokenizer’s original chat_template value and restore that exact value
during cleanup, rather than unconditionally setting it to None; alternatively,
use monkeypatch.setattr so pytest restores the attribute automatically.
- Around line 57-79: Update the q4_0 tolerance in test_weights_survive so it
scales relative to each tensor’s magnitude, or replace the absolute-only check
with a correlation-based assertion. Ensure the test still verifies that written
values match the model weights and fails for all-zero or unrelated tensors;
retain the existing f16 and q8_0 behavior.
---
Nitpick comments:
In `@src/bloomery/export.py`:
- Around line 243-245: Escape backslashes and double quotes in the eos value
before interpolating it into the quoted Modelfile stop parameter. Update the
export logic around eos and the lines.append call so arbitrary checkpoint
tokenizer values produce a valid Modelfile while preserving the existing
omission when no eos token is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fa9d7ba-9686-4a08-8767-107b809a701b
📒 Files selected for processing (13)
.github/workflows/ci.ymlREADME.mdpyproject.tomlsrc/bloomery/cli.pysrc/bloomery/export.pysrc/bloomery/jobs/runner.pysrc/bloomery/jobs/types.pysrc/bloomery/paths.pysrc/bloomery/server/static/app.jssrc/bloomery/server/static/index.htmlsrc/bloomery/train/loop.pytests/test_cli_train.pytests/test_export.py
Ten findings. Three were the same class as the missing merges: metadata that writes without complaint and produces a file no runtime will load. A mistral checkpoint was written with general.architecture = "mistral". llama.cpp registers this family once, under llama, and there is no such entry — the file would have failed with "unknown model architecture". Its tensor layout is Llama's, which is why the checkpoint is accepted at all; the name is not. Reported as mistral, written as llama. The token list was built by sorting the vocabulary, which reproduces the ids only when they are contiguous. Ours are; a published checkpoint's need not be, because they reserve blocks of ids and are routinely shorter than vocab_size. A gap shifted every later token onto the wrong id, and a short vocabulary left fewer entries than the embedding has rows. It is built by index now, sized to vocab_size, with unnamed rows filled. A checkpoint with neither an output projection nor an embedding raised KeyError past every handler. It is refused like everything else here. Never a moment with no checkpoint checkpoint.save deleted the old directory and then renamed the new one in. Between those calls there is nothing on disk, and a kill or a failed rename — across filesystems, say — takes the good checkpoint and puts nothing back. The old one is moved aside and discarded only once the new one is in place, with the move reversed if the rename fails. That also settles the export race properly. The plan noted that export runs concurrently with training and reads runs/<name>/latest while it is being replaced, and then did nothing about it. The window is what made that unsafe, and the window is gone, so the two can run together because of a property rather than a warning. The same fix is applied to export's own directory. Export cleanup ran only for ExportError, so a full disk or an interrupt mid-quantization left a partial GGUF in exports/<name>.tmp. It is a finally now, guarded by whether the result was placed. Tests The q4_0 round trip allowed an absolute error of 1.0 against weights whose standard deviation is about 0.02 — it would have passed a writer that emitted zeros, which is exactly what it exists to catch. Tolerances scale to each tensor's own spread now, with a correlation check that the values are that tensor and not merely something small. Verified by making the writer emit zeros and watching it fail. And a test mutated the shared tokenizer fixture, restoring the chat template to None rather than to what was there — the same fault flagged in M4.1, reintroduced. monkeypatch now. Also scoped a README claim to the runtimes this repo actually tests, and made the web form say that run and checkpoint are alternatives, which the CLI enforces and the form did not mention. Re-verified in Ollama after the metadata changes: still loads, still generates.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bloomery/export.py (3)
143-181: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNothing checks that the vocabulary size actually matches the embedding table before writing.
_vocabularysizes the token list fromconfig.vocab_size(or, if that is falsy, from the max token id in the tokenizer). It never compares that number against the real row count ofmodel.embed_tokens.weight/lm_head.weight. If a checkpoint'sconfig.vocab_sizeis stale relative to its actual embedding table — a known real-world Hugging Face pitfall after manual embedding resizes —to_ggufwrites a GGUF wheretokenizer.ggml.tokensandtoken_embd.weight's row count disagree, with noExportErrorraised.
tests/test_export.py::TestMetadataLlamaCppWillAccept::test_a_sparse_vocabulary_keeps_every_token_at_its_own_iddemonstrates this directly: it setsmodel.config.vocab_size = 10without touching the model's real embedding table, then callsto_gguf(model, tokenizer, out)at line 285, and the call does not raise even though the resulting file's token list length (10) would disagree with the real embedding row count.Add an explicit check comparing the vocabulary size against the embedding tensor's actual row count, and raise
ExportErroron mismatch, before writing any tokenizer or tensor metadata.🔧 Proposed fix
state = model.state_dict() if "lm_head.weight" not in state: embedding = state.get("model.embed_tokens.weight") if embedding is None: raise ExportError( "this checkpoint has neither an output projection nor a token " "embedding, so there is nothing to write as GGUF's output layer. " "It does not look like a causal language model." ) state = dict(state) state["lm_head.weight"] = embedding + embedding_rows = state.get("model.embed_tokens.weight", state["lm_head.weight"]).shape[0] + if config.vocab_size != embedding_rows: + raise ExportError( + f"config.vocab_size is {config.vocab_size} but the embedding table has " + f"{embedding_rows} rows; the GGUF this would write would disagree with " + "itself about the size of the vocabulary." + ) + # The official mapping rather than a hand-written table, so a rename upstream # is not something this has to notice. names = get_tensor_name_map(MODEL_ARCH.LLAMA, config.num_hidden_layers)Note: adding this check will make
test_a_sparse_vocabulary_keeps_every_token_at_its_own_id'sto_gguf(model, tokenizer, out)call (line 285) raise, since it deliberately desyncsconfig.vocab_sizefrom the real embedding row count. That test would need to resize the fixture's embedding table to match, or wrap the call inpytest.raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bloomery/export.py` around lines 143 - 181, In to_gguf, validate the effective vocabulary size produced by _vocabulary against the row count of the actual model.embed_tokens.weight or lm_head.weight tensor before creating GGUF metadata, including tokenizer metadata. Raise ExportError when the sizes differ, while preserving the existing fallback that derives lm_head.weight from the embedding tensor.
183-208: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTied embeddings get counted twice in
ExportResult.parameters.
state["lm_head.weight"] = embedding(line 158) aliases the same tensor object under a second key. The loop at line 186 iteratesstate.items()and addsarray.sizetoparametersfor bothmodel.embed_tokens.weightandlm_head.weight, even though they are the same weights. For every tied-embedding checkpoint — asserted true for the fixture model intest_the_output_head_is_written_though_it_is_tied— the reported parameter count is inflated by the size of the embedding matrix.This number reaches the user directly: the CLI prints it as
{format_params(result.parameters)} params, and--jsonserializes it inExportResult.to_dict().Track which tensor identities have already been counted, so a shared tensor written under two GGUF names is counted once.
🔧 Proposed fix
parameters = 0 written = 0 unquantized: list[str] = [] + counted: set[int] = set() for source, tensor in state.items(): mapped = names.get_name(source.removesuffix(".weight")) if mapped is None: continue name = f"{mapped}.weight" array = tensor.to(torch.float32).numpy() - parameters += int(array.size) + if id(tensor) not in counted: + counted.add(id(tensor)) + parameters += int(array.size)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bloomery/export.py` around lines 183 - 208, Update the parameter-counting logic in the state.items() loop so shared tensor identities are counted only once while each mapped tensor is still written under its own GGUF name. Track previously counted tensor objects using identity, and increment parameters only for the first occurrence; preserve the existing written count, quantization, and output-head behavior.
358-389: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMark placeholder vocabulary entries as
UNUSED._vocabularyemits[UNUSED_{index}]for missing IDs, but_add_tokenizermarks them asNORMAL. Missing ID1is therefore stored with type1, not GGUF type5. Track placeholder entries explicitly and emitTokenType.UNUSED; do not rely only on membership inadded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bloomery/export.py` around lines 358 - 389, Update _add_tokenizer so vocabulary entries matching the [UNUSED_{index}] placeholder format are emitted with TokenType.UNUSED (GGUF type 5), including missing IDs that are not in added. Preserve the existing added-token classification for non-placeholder entries, and use the project’s TokenType symbol rather than hard-coded numeric values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bloomery/train/checkpoint.py`:
- Around line 112-124: Update the checkpoint promotion flow around the staging
rename so the canonical directory remains resolvable after interruption: use an
atomic directory exchange when supported, otherwise implement stable indirection
with reader/writer coordination and crash recovery. Ensure recovery never
deletes the sole valid checkpoint, and retain `.previous` until a valid
canonical checkpoint is confirmed; preserve the guarantee that interrupted
writes cannot expose a half-written checkpoint.
---
Outside diff comments:
In `@src/bloomery/export.py`:
- Around line 143-181: In to_gguf, validate the effective vocabulary size
produced by _vocabulary against the row count of the actual
model.embed_tokens.weight or lm_head.weight tensor before creating GGUF
metadata, including tokenizer metadata. Raise ExportError when the sizes differ,
while preserving the existing fallback that derives lm_head.weight from the
embedding tensor.
- Around line 183-208: Update the parameter-counting logic in the state.items()
loop so shared tensor identities are counted only once while each mapped tensor
is still written under its own GGUF name. Track previously counted tensor
objects using identity, and increment parameters only for the first occurrence;
preserve the existing written count, quantization, and output-head behavior.
- Around line 358-389: Update _add_tokenizer so vocabulary entries matching the
[UNUSED_{index}] placeholder format are emitted with TokenType.UNUSED (GGUF type
5), including missing IDs that are not in added. Preserve the existing
added-token classification for non-placeholder entries, and use the project’s
TokenType symbol rather than hard-coded numeric values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 494c5e7f-9c43-4aa5-b742-3398c98fafe5
📒 Files selected for processing (8)
README.mdsrc/bloomery/cli.pysrc/bloomery/export.pysrc/bloomery/jobs/types.pysrc/bloomery/server/static/app.jssrc/bloomery/train/checkpoint.pytests/test_export.pytests/test_train.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bloomery/jobs/types.py
- src/bloomery/server/static/app.js
- README.md
- src/bloomery/cli.py
The fix in the previous commit narrowed the window where no checkpoint exists, and opened a worse path in doing so. A kill between the two renames leaves the good checkpoint at <name>.previous with nothing at <name> — and the next save began by deleting .previous to clear its way, destroying the only copy there was. The promotion is now preceded by a recovery: if the canonical path is missing and .previous holds a checkpoint, it is put back. Readers do the same, so `--resume` does not report nothing to resume while a complete checkpoint sits beside the path it looked at, and `export` finds one when it arrives mid-save — which it is built to do, since it runs concurrently with training by design. The window itself is not closed. An atomic directory exchange would do it and the syscall is Linux-only, so instead the state a kill leaves is made unambiguous and recoverable rather than impossible. That is the honest claim and it is what the docstring says. The test for this passed against the broken version, which is worth recording. A save that merely succeeds after an interruption hides the loss, because its own result replaces what was destroyed; only a save that then fails leaves nothing at all. The test makes the second rename raise and asserts the interrupted checkpoint is still there. It fails against the version without recovery.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bloomery/train/checkpoint.py`:
- Around line 135-154: Coordinate restore_interrupted() with the checkpoint
promotion logic in save(), using the same shared synchronization around checking
and renaming directory/.previous and removing .previous. Ensure concurrent
export recovery cannot move .previous before staging.rename(directory), and
avoid relying on an unsynchronized previous.exists() check before
shutil.rmtree(previous).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1df2f30-e09c-4f95-a5f6-3a4a7bb70022
📒 Files selected for processing (3)
src/bloomery/cli.pysrc/bloomery/train/checkpoint.pytests/test_train.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bloomery/cli.py
Adding recovery to readers in the previous commit created the race it was meant to guard against. A save mid-promotion is indistinguishable from an interrupted one — the canonical directory is absent and .previous holds a checkpoint — so a concurrent export doing the restore takes .previous out from under the save. The save's own rename then lands on a path that is suddenly occupied, and its recovery finds nothing to put back. A microsecond window became a failed training run, and export runs alongside training by design, so it is the command most likely to be there. Reading is safe; renaming is what races. `resolve` returns wherever the checkpoint can be read from and moves nothing, and is_resumable, load_resume_state and export use it. `restore_interrupted` still exists and is documented as the writer's alone. The exists-then-rmtree in save is now rmtree(ignore_errors=True): two operations against one path, and anything clearing it between them would raise FileNotFoundError out of an otherwise successful save. The test for this did not exist when I first claimed it did. The string replacement that was supposed to add it silently matched nothing, and five tests passing read as six. It asserts the directory listing is unchanged across a read, and fails against a reader that restores.
Two findings from the review that carried no comment id, so no thread ever
tracked them and both went unimplemented.
The vocabulary was sized from `config.vocab_size` and never checked against the
embedding it has to describe. llama.cpp builds `token_embd.weight` as
{n_embd, n_vocab} from the token list it read and rejects a file whose tensor
disagrees, so a stale config writes something whose own two halves contradict
each other. The two numbers match in a checkpoint nobody resized; a padded
embedding leaves the config low, and a resize on an older transformers leaves it
behind entirely. Both arrive through `adapt` on someone else's checkpoint.
Sized from the rows now. A token whose id is past the last row is refused rather
than dropped: dropping exports cleanly and crashes the first time the tokenizer
produces one. The reported vocab is what was written rather than what was
claimed.
The Modelfile interpolated the eos token into a quoted value with no escaping,
so a token containing a quote ended the value early and left the rest as stray
arguments. It is left out rather than escaped — whether that grammar reads a
backslash as an escape is not something this verifies, and a stop parameter that
is quietly the wrong string is worse than an absent one. The eos id travels in
the GGUF metadata, which is what a runtime stops on regardless.
Each of the three tests was run against the old behaviour first. The malformed
line they produce is `PARAMETER stop "<|end"of"text|>"`.
|
Two findings from the last two review bodies were never implemented, and neither had a thread to notice that. The nitpick on the Modelfile eos token and the outside-diff comment on vocabulary sizing both arrived without a Vocabulary sizing was the real one. The rows are the truth, so the list is sized from them. A token whose id is past the last row is now refused rather than dropped — dropping exports cleanly and crashes the first time the tokenizer produces one. You were also right that The Modelfile token is left out rather than escaped. The proposed Three tests, each run against the old behaviour first — the malformed line it produces is 🤖 Addressed by Claude Code |
1 participant
M5.
bloomery exportwrites a GGUF and an Ollama Modelfile, at f16, q8_0 or q4_0.Why this is written here rather than shelled out
llama.cpp's
convert_hf_to_gguf.pyidentifies a tokenizer by hashing its output against a table of known models. A tokenizer this project trained is a new hash by construction — measured74a7f913c36b5d2fon a real checkpoint, which is in no table anywhere. The stock path refuses precisely the from-scratch models bloomery leads with.Refusing is correct for a general tool: the hash picks the pre-tokenizer regex, and the wrong one produces a model that loads and talks nonsense. But bloomery holds the answer that fingerprint is trying to recover — its tokenizers are byte-level BPE with the standard regex — so the field is set outright.
The README has claimed the opposite since before any of this existed: "GGUF conversion, vLLM and Ollama all work without a bespoke converter." It now says what is true.
The round trip passed while the file was unloadable
Tensors, vocabulary, metadata and the tied output head were all verified by reading the file back, at every quantization. All of it passed — and Ollama refused the result:
BPE is its merge rules, not just its vocabulary. I wrote the token list and not the merges. That was found by running an export through Ollama, not by any test in this PR, and there is now a regression test that fails without the merges.
Verified end to end after the fix:
ollama createaccepts the file, and the model generates —"elle theen morning kitchen in sh: lostet gla l'ex", 16 tokens,done_reason: length. Gibberish, because that checkpoint had 6 training steps on synthetic data; the point is that it runs.Details worth recording
lm_head. Loading materialises one; it is derived from the embedding when it does not.ggufpackage raisesNotImplementedErrorwhen asked to write one. It can read them, which is a different thing. The command namesllama-quantizerather than pretending.context_lengthis the sequence length the model was trained at, not a capability, and a runtime holds it as a hard limit. It is reported at export time rather than discovered later.exportjob kind is wired through all five mirrors and deliberately not inEXCLUSIVE_KINDS— it is CPU and disk work, so it need not queue behind a training run.A pre-existing flake, found and fixed
The memory pre-flight derives its budget from available RAM, and a depth-1 model estimates ~0.8 GiB once fixed overhead is counted. On a constrained machine that budget dips below the estimate partway through a suite run, and
trainoradaptrefuses — so a test about tokenizers fails for reasons of its host.It surfaced as this file passing alone and failing inside the full suite, with a different test each time. 32 of 34
train/adaptinvocations were exposed. The budget is now pinned for CLI tests, withTestMemoryGuardopting out because it is what tests the real one. Three consecutive full-suite runs clean.Verification
664 tests, plus ruff, mypy and the JavaScript checks. No network and no llama.cpp needed: the suite's own from-scratch checkpoints are the fixtures, and the round-trip test is what removes the dependency on having a runtime installed.
Not in this PR
llama-quantizereads it.adapt --method fullon a Qwen base produces one, and a wrong tensor mapping yields a file that loads and generates nonsense.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes