fix(config): an absent field is None, not a plausible number by ciresnave-bot · Pull Request #77 · ciresnave/mlmf

ciresnave-bot · 2026-09-11T03:47:58Z

Closes #48. Implements CireSnave's standing policy, recorded verbatim in CLAUDE.md §2 and quoted in full there:

"Make the fields a supported format may not supply Option<T>. That way it doesn't matter if they are unable to supply them or someone previously writing the file chose not to write those fields to the file, either way MLMF just works."

The discriminator, which is sharper than "documented default"

A specification that says what ABSENCE MEANS licenses a value. One that merely offers a convenient starting number does not.

That test decides every field, and it is checkable rather than a matter of taste. The constants removed here fail it in their own comments, verbatim before removal:

4096,                 // Common LLaMA max length
self.hidden_size * 4, // SwiGLU typically uses 4x hidden_size

"Common" and "typically" are statements about what models usually do. The HF config format documents no meaning for an absent key at all.

num_key_value_heads passes the test and therefore stays concrete: absent genuinely means one KV head per query head — ordinary multi-head attention, which is what GQA degenerates to. Turning it into None would discard a fact the format actually gives.

Eight fields become `Option<T>`

intermediate_size · max_position_embeddings · dropout · layer_norm_eps · attention_dropout · activation_function · rope_theta · tie_word_embeddings

⚠️ On the HuggingFace path this was never missing information — it was DESTROYED information. serde already declared twelve of these Option; to_model_config collapsed them one layer later through nine unwrap_or calls. Eight are gone, and the type is what keeps them gone. The ninth is the licensed one above.

Consumers, where an absent input must not quietly become a fabricated one

estimate_memory_usage returns Option<MemoryEstimate>. ⚠️ An under-estimate is the dangerous direction: too small does not look wrong, it looks affordable, and the caller allocates against it. So every assumption errs toward MORE memory. A finite domain takes its worst case (gating, tying — both booleans, so the bound is exact); an unbounded integer refuses. GGUF requires feed_forward_length, so every GGUF model still estimates.
validate_memory_requirements refuses rather than returning Ok. A check that could not run must not report success — a caller cannot distinguish "validated" from "skipped".
The model card renders not declared, and emits no 7b size tag from a count it could not compute. A tag is quotable metadata that outlives the card.
is_gated_ffn returns Option<bool>. ⚠️ Its sibling ffn_hidden_size had two arms returning the same expression, so the gated branch was dead and its comment described behaviour the code did not have. Removed rather than "fixed" — doubling the value would be MLMF deciding what a gated FFN's hidden size is, which is a consumer's call.

⚠️ This commit nearly retired the guard that exists for this defect class

model_config_literals.rs scans ModelConfig constructions for invented literals. Every value it inspects changed shape in one commit: rope_theta: 10000.0 became rope_theta: Some(10000.0). A Some(..) reads as a call, calls are expressions, and that scanner permits expressions — so the change that removed the defect would also have made the defect invisible. Its own doc had named the hazard: "every form this fails to recognise is a SILENT PASS." It arrived by a route nobody anticipated, because the FIX rewrote the syntax rather than a person.

The only thing that surfaced it was the file's non-vacuity assert firing.

Control — same sabotage, same tree, one variable:

`is_literal`	on a planted `rope_theta: Some(10000.0)`
with the `Some(..)` unwrap	FAILED, naming `gguf.rs:703`
without it	ok — the fabricated constant passes undetected

⚠️ Its non-vacuity assert also had to change: it required literal_fields > 0, which #48 made unsatisfiable. The old message advised deleting the guard as having no population. That would have been wrong. The class is not dead, it MOVED: onnx_import.rs still seeds GPT-2's 50257 / 768 / 12 in let mut initialisers, one syntactic step outside what the scanner reads. The assert now counts model fields parsed rather than literals found — zero iff the parser is broken, positive on a clean tree — with the literal half proven by this file's own unit tests, which cannot be silenced by the tree changing shape.

ONNX

max_position_embeddings was seeded to 2048 and never assigned anywhere, so every ONNX model MLMF has ever loaded reported a 2048 context length unconditionally — not even as a fallback. Now None. intermediate_size (GPT-2's 3072) likewise.

⚠️ The remaining GPT-2 seeds are #76 and deliberately NOT in this PR. vocab_size / hidden_size / num_hidden_layers are supplied by HF and GGUF (28/28 measured), so they are correctly concrete; ONNX needs a refusal, not an Option. Burying a behaviour change inside a type change is how it ships unreviewed. num_attention_heads is deferred with it — it is derived by division, and head_dim() has 11 read sites and needs its own answer.

⚠️ Verification, and the scope limit that matters most

CI does not test or lint the root mlmf package, and that is deliberate and documented in ci.yml (it needs protoc, and spec §11 schedules it for rewrite). Control: cargo (test|clippy|build) -p mlmf- appears 68 times in that workflow; no step names the root package or --workspace. Every change in this PR except model_config_literals.rs lives there, so a green CI tick is not evidence about it. The local runs below are the evidence.

Taken at origin/main 93e7401:

fmt --all --check   clean          110 lib tests        29 doc tests
8/8 gated crates    test + clippy -D warnings clean

Root-crate clippy: 0 of 313 warnings land on a line this change authored, across 572 added lines. Control: config.rs has 303 added lines and 4 warnings (279, 421, 229, 488) — both populations present, zero overlap.
Sabotage, two orthogonal: reintroducing the rope_theta fallback reddens both new tests; so does an independent one on intermediate_size.
The corpus test was made to speak rather than assumed to run: changing its expected value produced llama.rope.freq_base, got 100000 from a real checkpoint — where the removed default was 10000, the factor of ten fix(gguf): read the model config from the file instead of fabricating it #37 measured.

The two new tests, and why there are two

a_declared_value_and_a_silent_file_are_distinguishable declares exactly the constant the fallback invents. Any other fixture value would differ today and pass straight over the live defect; declaring the fallback's own number makes the two files identical in the output, which is the §6 tell CLAUDE.md names — "a caller cannot distinguish 'the model uses 10000' from 'MLMF had nothing to say.'" It failed red on left: 10000.0, right: 10000.0.

an_absent_key_is_none_and_a_declared_one_keeps_its_value is not redundant: assert_ne! alone would be satisfied by a conversion returning Some(9999.0) for the silent file. It names both sides exactly, and pins that num_key_value_heads did not move.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MdVuiraXRfDHQ227cjBt51

Summary by Sourcery

Represent unsupported or undisclosed model configuration fields as optional values and make downstream consumers refuse or clearly report unknown results instead of inventing model metadata.

Bug Fixes:

Preserve absent optional configuration fields as None instead of fabricating architecture-specific or format-independent values.
Prevent memory validation, parameter estimates, model-card metadata, and ONNX configuration from reporting misleading results when required information is unavailable.
Keep the documented num_key_value_heads absence behavior while correcting GGUF and HuggingFace field handling.

Enhancements:

Update configuration consumers and examples to handle optional values explicitly, using conservative memory-estimation assumptions only for bounded unknowns.
Strengthen the model configuration literal guard so wrapped constants remain detectable and scanner liveness cannot become vacuous.

Tests:

Add coverage proving declared and absent configuration values remain distinguishable and that documented concrete defaults are preserved.

CireSnave's standing policy, recorded verbatim in CLAUDE.md section 2: "Make the fields a supported format may not supply `Option<T>`. That way it doesn't matter if they are unable to supply them or someone previously writing the file chose not to write those fields to the file, either way MLMF just works." Eight `ModelConfig` fields become `Option<T>`: intermediate_size, max_position_embeddings, dropout, layer_norm_eps, attention_dropout, activation_function, rope_theta, tie_word_embeddings. The discriminator is sharper than "documented default": a specification that says what ABSENCE MEANS licenses a value; one that merely offers a convenient starting number does not. Every constant removed here failed that test -- their own comments called 4096 a "Common LLaMA max length" and hidden_size * 4 what SwiGLU "typically" uses. `num_key_value_heads` passes it, because absent genuinely means one KV head per query head, so it stays concrete. On the HuggingFace path this was not missing information but DESTROYED information: serde already declared twelve of these `Option`, and `to_model_config` collapsed them one layer later with nine `unwrap_or` calls. Eight are gone; the ninth is the licensed one. Consumers, where an absent input must not become a fabricated one: - `estimate_memory_usage` returns `Option<MemoryEstimate>`. Every assumption it makes errs toward MORE memory, never less -- a finite domain (gating, tying) takes its worst case, an unbounded integer (intermediate_size) refuses. GGUF requires feed_forward_length, so every GGUF model still estimates. - `validate_memory_requirements` refuses rather than returning Ok. A check that could not run must not report success. - The model card renders "not declared" instead of a number, and emits no "7b" size tag from a count it could not compute. - `is_gated_ffn` returns `Option<bool>`. Its sibling `ffn_hidden_size` had two arms returning the same expression, so the gated branch was dead and its comment described behaviour the code did not have; the branch is removed rather than "fixed", which would have meant deciding a consumer's question. ONNX: `max_position_embeddings` was seeded to 2048 and never assigned anywhere, so every ONNX model reported a 2048 context length unconditionally. Now None. The remaining GPT-2 seeds are #76. model_config_literals.rs: `is_literal` now sees through `Some(..)`. Without it this commit would have silently retired that guard -- every value it inspects changed shape at once, and `Some(10000.0)` reads as a call, which the scanner permits. Verified by control: with the unwrap the planted `rope_theta: Some(10000.0)` is caught; without it the same sabotage passes. Its non-vacuity assert now counts model fields PARSED rather than literals FOUND, because the old form became unsatisfiable once the defect was gone -- and deleting the guard, which its own message advised, would have retired the only mechanical check on a class that is still live in onnx_import.rs. Verified: fmt --all clean; 110 lib tests; 29 doc tests; all 8 gated crates test + clippy -D warnings clean; 0 of 313 root-crate clippy warnings land on a line this change authored (control: config.rs has 303 added lines and 4 warnings, none overlapping). Sabotages: reintroducing the rope_theta fallback reddens both new tests, and so does an orthogonal one on intermediate_size. The corpus test was made to speak -- it reports `llama.rope.freq_base, got 100000` from a real checkpoint, where the removed default was 10000. Closes #48 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MdVuiraXRfDHQ227cjBt51

sourcery-ai

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 2 hours by commenting @sourcery-ai review. Upgrade to get a review now.

sourcery-ai · 2026-09-11T03:48:17Z

Reviewer's Guide

The PR removes unsupported configuration defaults and carries absence as Option<T> through format conversion, memory validation, and model-card generation, while strengthening tests and the literal scanner so fabricated values cannot be hidden by the new syntax or by vacuous checks.

Sequence diagram for preserving absent configuration fields

sequenceDiagram
    participant File as Model file
    participant Parser as Format parser
    participant Converter as Format converter
    participant Config as ModelConfig
    participant Consumer as Consumer

    File->>Parser: parse configuration
    Parser->>Converter: optional field or None
    Converter->>Config: to_model_config(architecture)
    Config-->>Consumer: declared value or None
    alt intermediate_size absent
        Consumer->>Config: estimate_memory_usage()
        Config-->>Consumer: None
    else intermediate_size declared
        Consumer->>Config: estimate_memory_usage()
        Config-->>Consumer: Some(MemoryEstimate)
    end

Flow diagram for absence-aware consumers

flowchart LR
    Config["ModelConfig with Option fields"] --> Estimate{intermediate_size available?}
    Estimate -->|No| NoEstimate["No MemoryEstimate"]
    Estimate -->|Yes| Memory["estimate_memory_usage returns Some"]
    NoEstimate --> Validation["validate_memory_requirements refuses"]
    Memory --> Validation
    Config --> Params{parameter count computable?}
    Params -->|No| Card["Model card: not declared; no size tag"]
    Params -->|Yes| Card2["Model card: render count and size tag"]
    Config --> Sequence{max position declared?}
    Sequence -->|No| Length["Max sequence length: not declared"]
    Sequence -->|Yes| Length2["Render declared length"]

File-Level Changes

Change	Details	Files
Preserve absent optional configuration fields as `None` instead of synthesizing architecture- or format-based values.	Changed eight `ModelConfig` fields to `Option<T>` and removed HuggingFace fallbacks. Preserved the documented `num_key_value_heads` absence semantics. Updated HuggingFace, GGUF, and ONNX conversions to retain declared values and represent unsupported absence explicitly. Removed the unassigned ONNX context-length seed and made FFN-size inference optional.	`src/config.rs` `src/formats/gguf.rs` `src/formats/onnx_import.rs`
Propagate uncertainty through model consumers rather than reporting fabricated measurements.	Made memory estimation return `Option<MemoryEstimate>` and refuse unbounded FFN-size cases. Made memory validation fail when estimation cannot run. Made parameter counts and maximum sequence lengths optional in model cards, suppressing size tags when counts are unavailable. Updated examples and call sites for the new optional APIs.	`src/validation.rs` `src/model_card.rs` `examples/load_llama.rs` `examples/model_card_example.rs` `examples/test_gqa_memory.rs`
Harden the literal-field regression guard against syntax changes and vacuous success.	Taught the scanner to inspect constants wrapped in `Some(...)`. Changed non-vacuity tracking to count parsed model fields rather than requiring production literals. Retained unit coverage for literal detection and added conversion tests distinguishing declared fallback values from silent fields.	`crates/mlmf-core/tests/model_config_literals.rs` `src/config.rs`

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#48	Make model-specific fields that a format cannot supply representable as absent rather than replacing them with fabricated defaults, including fields such as rope_theta, activation_function, and the ONNX-derived attention-head metadata.	❌	The PR correctly converts many fields to Option and removes several fabricated ONNX values, but it explicitly leaves num_key_value_heads concrete and continues to assign it from the guessed num_heads. num_attention_heads and other ONNX seeds also remain concrete. Thus ONNX models can still report guessed head metadata and appear non-GQA, which the issue identifies as the most serious remaining defect.
#48	Preserve the distinction between declared and absent values through loaders and ModelConfig construction, including the GGUF and Hugging Face paths.	❌	The GGUF and most Hugging Face conversions now preserve absence for the affected optional fields, and tests cover declared versus silent values. However, HFConfig still applies a serde default of 0.1 to dropout before conversion, so a missing dropout remains indistinguishable from an explicitly declared 0.1. This is less critical because the issue permits honest inference-time defaults, but it means the broader absence-preservation objective is not complete.
#48	Ensure consumers do not turn absent configuration values back into misleading estimates or metadata.	✅

Possibly linked issues

ModelConfig cannot represent an absent field, so loaders invent one #48: The PR directly resolves issue ModelConfig cannot represent an absent field, so loaders invent one #48 by replacing fabricated defaults with Option fields and updating loaders and consumers.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

codacy-production · 2026-09-11T03:48:49Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity · 2 duplication

Metric Results

Complexity 9

Duplication 2

View in Codacy

_{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.}

sourcery-ai Bot reviewed Sep 11, 2026

View reviewed changes

ciresnave merged commit a4270c8 into main Sep 11, 2026
6 checks passed

ciresnave deleted the an-absent-field-is-none-not-a-plausible-number branch September 11, 2026 03:57

Navigation Menu

fix(config): an absent field is None, not a plausible number - #77

fix(config): an absent field is None, not a plausible number#77
ciresnave merged 1 commit into
mainfrom
an-absent-field-is-none-not-a-plausible-number

ciresnave-bot commented Sep 11, 2026 •

edited by sourcery-ai Bot

Loading

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

sourcery-ai Bot commented Sep 11, 2026

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

codacy-production Bot commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ciresnave-bot commented Sep 11, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The discriminator, which is sharper than "documented default"

Eight fields become Option<T>

Consumers, where an absent input must not quietly become a fabricated one

⚠️ This commit nearly retired the guard that exists for this defect class

ONNX

⚠️ Verification, and the scope limit that matters most

The two new tests, and why there are two

Summary by Sourcery

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot commented Sep 11, 2026

Reviewer's Guide

Sequence diagram for preserving absent configuration fields

Flow diagram for absence-aware consumers

File-Level Changes

Assessment against linked issues

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

codacy-production Bot commented Sep 11, 2026

Up to standards ✅

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ciresnave-bot commented Sep 11, 2026 •

edited by sourcery-ai Bot

Loading

Eight fields become `Option<T>`