Restore model coverage gates - #647
Merged
Merged
Conversation
Read L2 test IDs from the finalized registry, cover legacy aliases with their canonical checkpoints, and document internal/GGUF routes that cannot have independent Hugging Face config or golden cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: justinchuby <justinchuby@users.noreply.github.com>
Performance Comparison
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores the model coverage gates by shifting L2 (“has test_model_id”) coverage checks to use the finalized ModelRegistry registrations (rather than a test-only map), while also bringing legacy alias model types (Bailing/DeepSeek) back under the same canonical HF checkpoint evidence and documenting routes that can’t own independent HF configs/golden cases.
Changes:
- Update
tests/model_coverage_test.pyL2 validation to derivetest_model_idfromregistryregistrations and expand skip-list documentation for internal/GGUF-only routes. - Add
test_model_identries forbailing_moeanddeepseekinsrc/mobius/_registry.pyto match their canonical checkpoints.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/model_coverage_test.py |
Switch L2 coverage source to registry registrations; expand coverage skip documentation for alias/GGUF-only routes. |
src/mobius/_registry.py |
Add missing test_model_id mappings for legacy aliases (bailing_moe, deepseek) to restore L2 gating. |
Suppressed comments (1)
tests/model_coverage_test.py:450
- The aggregate L2 failure message still tells contributors to update _TEST_MODEL_IDS, but the test logic now checks for a registered ModelRegistration.test_model_id. Updating the message to refer to the registered field (and optionally mention that it is currently populated via _TEST_MODEL_IDS) will make failures less confusing.
f"{len(missing)} registered model(s) have no "
f"test_model_id in _registry.py and are not in "
f"_COVERAGE_SKIP:\n"
+ "\n".join(f" {mt}" for mt in missing)
+ "\n\nFix: add test_model_id to _TEST_MODEL_IDS "
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
123
to
+127
| def _all_registered_with_test_id() -> dict[str, str]: | ||
| """Return {model_type: test_model_id} for registered models with one.""" | ||
| return { | ||
| arch: model_id for arch, model_id in _TEST_MODEL_IDS.items() if arch in registry._map | ||
| arch: registration.test_model_id | ||
| for arch, registration in registry._map.items() |
Point L2 coverage failures at the finalized model registration field now used by the checks, rather than implying every identifier must come from the legacy helper map. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: justinchuby <justinchuby@users.noreply.github.com>
justinchuby
added a commit
to justinchuby/onnx-genai
that referenced
this pull request
Aug 26, 2026
…2171) ## Summary - establish `package.tokenizer.special_tokens` as the sole package authority for execution-relevant numeric token IDs - preserve ordered multi-EOS defaults while treating request EOS values as replacement overrides - require complete-generation workflows to execute termination semantics; keep logits-only packages explicitly valid without sampler/termination claims - materialize one effective token set for workflow graphs and fused/native runtime paths - document capability obligations, portable state invariants, policy graphs, ONNX ABI versus workflow authority, and reserved versus extensible identifiers - validate and publish schema-v1.2 canonical plus annotated metadata across the hosted collection ## Contract Token IDs are package facts under `package.tokenizer.special_tokens`. Token spellings, added-token mappings, and chat templates remain authoritative in tokenizer assets. Workflow ports route semantic values but do not duplicate ONNX physical ABI, and termination policy consumes resolved token facts without owning another numeric copy. Request EOS values replace package defaults when supplied. Pre-v1.2 compatibility may derive EOS from legacy tokenizer assets; v1.2 packages fail closed on retired workflow/package EOS literals. ## Reader documentation - [Metadata capability model](https://github.com/justinchuby/onnx-genai/blob/justinchuby/special-token-authority/docs/genai/METADATA_CAPABILITY_MODEL.md) - [Normative inference metadata decisions](https://github.com/justinchuby/onnx-genai/blob/justinchuby/special-token-authority/docs/genai/INFERENCE_METADATA_DECISIONS.md) - [Reader tracking issue #2143](#2143) ## Producer and hosted examples - Mobius producer migration: onnxruntime/mobius#644 - Mobius baseline coverage repair: onnxruntime/mobius#647 - Hosted collection: **28 repositories / 56 canonical+annotated files**, including 12 complete-generation packages migrated to schema v1.2 - Catalogue revision: [`8ba416600109201e10256841e20f0c1d2777af6e`](https://huggingface.co/datasets/justinchuby/onnx-genai-inference-metadata-catalogue/tree/8ba416600109201e10256841e20f0c1d2777af6e) ## Validation - metadata and genai-config test suites - engine authored-workflow and multi-EOS tests - ORT tokenizer/chat-template tests - server multimodal tests and Rust 1.98 Clippy with warnings denied - generated schema synchronization, formatting, and `git diff --check` - collection validator: semantic YAML equality, inline comments, README links, provenance hashes, generation scope, token authority, executable termination, schema, and shapes for all 56 files The non-required **Mobius metadata packages (signal)** job is expected to reject current Mobius `main` fixtures until producer PR #644 lands; v1.2 validation is intentionally not weakened for that transition. --------- Signed-off-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Reviewer handoff: this focused baseline coverage repair is green, has no architecture or performance delta, and has auto-merge enabled. It is the only prerequisite blocking cleanup of the stacked metadata producer PR #644; requesting the required human approval when available. |
Member
Author
|
CI blocker diagnosis: this PR's As of 2026-08-26 13:20 UTC:
I am not rerunning the workflow because that would only add queue pressure and reset this PR's position. Auto-merge remains enabled; the remaining gates are this shared A10 check and the requested |
justinchuby
added a commit
that referenced
this pull request
Aug 26, 2026
## Summary - emit execution-relevant numeric token IDs once under `package.tokenizer.special_tokens` - preserve ordered multi-EOS package defaults while making workflow EOS tensors runtime-bound request overrides - remove authored `package.eos_ids` and `package.eos_token_ids` literals from decoder and VLM workflows - cover image, video, audio, and vision-start placeholders; restrict CTC metadata to relevant blank/padding semantics - stamp schema v1.2 independently of tokenizer availability and retain public `SpecialTokenFact` compatibility - refresh decoder, static-cache, and VLM conformance fixtures ## Contract Token IDs are package facts. Token spellings, added-token mappings, and chat templates remain authoritative in tokenizer assets. Termination policy consumes resolved package defaults or an explicit request replacement but owns no duplicate IDs. Consumer schema/runtime: justinchuby/onnx-genai#2171 (**merged** as `e599dc0f067d35aa5326ce5c97d83d2fd6d02f78`) Reader guide: justinchuby/onnx-genai#2143 Baseline coverage repair: #647 ## Validation - **1,095 passed / 250 skipped** across `src/mobius/integrations/onnx_genai` and `tests/model_coverage_test.py` - full `lintrunner` - generated schema and fixture validation - `git diff --check` - architecture and benchmark checks report no graph or performance regression - merged consumer validation passes **28/28 hosted repositories and 56/56 metadata files** This branch is temporarily stacked on #647 solely to avoid known current-`main` coverage failures. After #647 merges, rebase this PR onto current `main` so only the two producer commits remain, rerun the focused gates, and merge. --------- Signed-off-by: justinchuby <justinchuby@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
Validation
python -m pytest tests/model_coverage_test.py -q --tb=shortlintrunner --output oneline --all-filesThis restores the currently failing main-branch test matrix and unblocks dependent PRs.