fix(loader): a file named config.json must hold a config, or not exist - #75
Merged
Merged
Conversation
Closes #72. `save_with_metadata` -- a PUBLIC API -- wrote `raw_config` to a file called `config.json` unconditionally. Only the HuggingFace loader has a source config to copy; GGUF and ONNX set `raw_config` to `Value::Null`. Measured on a real GGUF checkpoint whose configuration MLMF had read CORRECTLY: config.architecture = LLaMA <- read from the file config.vocab_size = 49152 <- read from the file config.json = null <- 4 bytes, written to disk⚠️ A downstream reader gets `null`, not an error: a write path producing a plausible artifact with no data, with the data in hand on the same struct. That is the severe half of the class this crate has spent its recent history removing from the READ side (#37, #43, #50, #58, #59, #62). ## The file is now omitted, not faked An absent `config.json` says the true thing -- this source format has no config file -- and `metadata.json` still carries what MLMF read.⚠️ DELIBERATELY NOT "serialise `ModelConfig` instead". That would put MLMF's NORMALISED VIEW under a name that, on the HF path, means "the bytes the model shipped with". One filename, two provenances, and a consumer unable to tell which it holds -- solving this by manufacturing #48's problem inside the field meant to fix it. The omission is announced on stderr rather than silent: a caller gets one file fewer than the HF path produces, and silence about that is how someone builds a loader that trips over the missing name later. ## The second consumer: every GGUF and ONNX model shared one config_hash `serde_json::to_string(&Value::Null)` is the four characters `"null"`, so the hash was IDENTICAL across two entire formats -- a collision by construction in a field whose only purpose is telling two models apart. `update_hashes` already takes `Option<String>` and the field is already `Option`, so "there is no config to hash" was representable all along; the code declined to say it.⚠️ NOT REPLACED WITH A HASH OF `ModelConfig`'S FIELDS, AND I CHECKED BEFORE DESIGNING ONE. `config_hash` is written here, stored on `ModelMetadata`, and READ BY NOTHING in this workspace. Inventing a hashing scheme -- and choosing between a hand-written field list that drifts silently and a `Debug` string that is unstable across compilers -- would have been picking trade-offs on behalf of nobody. If a consumer appears it can say what it needs. ## Tests: two arms, ONE VARIABLE Both tests load the SAME HuggingFace fixture and set `raw_config` to `Null` on one arm. A GGUF fixture would have varied the format, the loader AND the tensor set at once, and a difference in the output would not have named its cause. no_source_config_means_no_config_json_and_no_config_hash an_absent_source_config_yields_no_hash_rather_than_a_shared_one SABOTAGES -- each reverts one half, and hits ONLY the test for that half: write config.json unconditionally config-json test RED hash test green hash raw_config unconditionally config-json test green hash test RED⚠️ AND THE TEST CAUGHT MY OWN WRONG MODEL OF THE CODE. My first draft asserted that a freshly-loaded model whose `raw_config` I had nulled would have `config_hash == None`. It does not: `load_safetensors` calls `update_model_hashes` DURING the load, so no model reaches a test unhashed. The surviving assertions are stronger for it -- one pins that the real hash is KEPT rather than overwritten, the other names the exact value that would have replaced it, so a failure says WHICH defect returned. VERIFIED: cargo clippy --workspace --all-targets --all-features exit 0 cargo fmt --all --check exit 0 cargo test -p mlmf --lib 108 passed cargo test -p mlmf --doc 29 passed cargo test -p mlmf-core 132 passed, 18 binaries Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdVuiraXRfDHQ227cjBt51
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ciresnave-bot, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 5 days and 7 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideUpdates save_with_metadata and hash generation so formats without a source config no longer emit a misleading config.json or a collision-prone hash of JSON null, while retaining HuggingFace source-config behavior and adding focused regression tests. Flow diagram for source-config-aware model savingflowchart TD
A[save_with_metadata] --> B{raw_config is null?}
B -->|No| C[Write source config to config.json]
B -->|Yes| D[Omit config.json]
D --> E[Write metadata.json and tensor_mappings.json]
C --> E
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
2 participants
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.
Closes #72.
save_with_metadata— a public API — wroteraw_configto a file calledconfig.jsonunconditionally. Only the HuggingFace loader has a source config tocopy; GGUF and ONNX set
raw_configtoValue::Null.Measured on a real GGUF checkpoint whose configuration MLMF had read correctly:
null, not an error. A write path producing aplausible artifact with no data, with the data in hand on the same struct —
the severe half of the class this crate has spent its recent history removing from
the read side (#37, #43, #50, #58, #59, #62).
The file is omitted, not faked
An absent
config.jsonsays the true thing: this source format has no configfile.
metadata.jsonstill carries what MLMF read.ModelConfiginstead". That would putMLMF's normalised view under a name that, on the HF path, means the bytes the
model shipped with. One filename, two provenances, and a consumer unable to tell
which it holds — solving this by manufacturing #48's problem inside the field
meant to fix it.
The omission is announced on stderr rather than silent: a caller gets one file
fewer than the HF path produces, and silence about that is how someone builds a
loader that trips over the missing name later.
The second consumer: one hash shared by two whole formats
serde_json::to_string(&Value::Null)is the four characters"null", soconfig_hashwas identical for every GGUF and every ONNX model — a collisionby construction, in a field whose only purpose is telling two models apart.
update_hashesalready takesOption<String>and the field is alreadyOption,so "there is no config to hash" was representable all along.
ModelConfig's fields — and I checked beforedesigning one.
config_hashis written here, stored onModelMetadata, andread by nothing in this workspace. Inventing a scheme would have meant choosing
between a hand-written field list that drifts silently and a
Debugstring that isunstable across compilers — trade-offs on behalf of nobody. If a consumer
appears, it can say what it needs.
Tests: two arms, one variable
Both load the same HuggingFace fixture and set⚠️ A GGUF fixture would have varied the format, the loader and the tensor
raw_configtoNullon onearm.
set at once, and a difference in the output would not have named its cause.
config.jsonunconditionallyraw_configunconditionallyEach reverts one half and hits only that half's test.
My first draft asserted that a freshly-loaded model whose
raw_configI had nulledwould have
config_hash == None. It does not —load_safetensorscallsupdate_model_hashesduring the load, so no model reaches a test unhashed.The surviving assertions are stronger for it: one pins that the real hash is kept
rather than overwritten, the other names the exact value that would have
replaced it, so a failure says which defect returned rather than only that
something changed.
Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01MdVuiraXRfDHQ227cjBt51
Summary by Sourcery
Ensure saved model artifacts accurately represent whether the source format provided a configuration file and configuration hash.
Bug Fixes:
config.jsoncontainingnullwhen the source format has no configuration file.Enhancements:
null-based hash collisions.Tests: