Clean up GGUF review findings - #633
Merged
Merged
Conversation
Resolve the still-valid low-priority typing, documentation, test robustness, error-message, maintainability, and reuse-performance findings left across the GGUF PR stack. Preserve the merged behavioral contracts while reducing GGUF save hashing to one final integrity check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Performance Comparison
|
🏗️ Architecture Diff
No architecture changes detected. ✅ Legend: ⚪ No change · 🔵 Minor (attrs/inits) · 🟡 Moderate (nodes added/removed) · 🔴 Major (interface changed) |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR cleans up remaining low-priority review findings from the GGUF PR stack while preserving the already-merged behavioral fixes. The changes primarily tighten documentation, improve test robustness, and refine GGUF reuse/package integrity checks so multi-GB sources are hashed once at the final verification gate.
Changes:
- Harden GGUF reuse/publishing flows and related tests (single final SHA-256 verification; cheaper identity checks during staging).
- Improve test robustness by removing brittle assertions (node counts, full-route equality) and tightening runtime-evidence filtering.
- Documentation/typing cleanups across tasks, GGUF contract validation, and model components.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/yaml_schema_test.py | Filter required runtime routes to ORT GenAI evidence only. |
| tests/synthetic_parity_test.py | Remove stale divergence commentary for newly registered models. |
| tests/ort_genai_e2e_test.py | Make expected ORT GenAI version env override default to pinned value. |
| tests/gguf_small_model_runtime_integration_test.py | Stabilize import-route assertions; improve ORT GenAI version resolution. |
| tests/e2e_golden_test.py | Avoid private GGUF reader internals; make tensor/qtype checks API-driven. |
| testdata/cases/schema.json | Restore schema tail formatting/indentation consistency. |
| src/mobius/tasks/_ssm_causal_lm.py | Clarify sequence_length semantics for Mamba-1 vs Mamba2. |
| src/mobius/tasks/_dflash.py | Update DFlash outputs contract docs for conditional draft logits/hidden. |
| src/mobius/tasks/_causal_lm.py | Fix canonical issue URL format for runtime-support metadata. |
| src/mobius/tasks/_cache_utils.py | Expand function-registration docs to include Mamba-1 layers. |
| src/mobius/models/t5_test.py | Remove brittle node-count assertions from graph contract test. |
| src/mobius/models/qwen3_tts_tokenizer.py | Correct waveform shape docs for multichannel audio. |
| src/mobius/models/plamo2.py | Replace hard-coded INT64 sentinel with shared INT64_MAX. |
| src/mobius/models/glm_moe_dsa_test.py | Make cache IO selection robust by matching port name prefixes. |
| src/mobius/models/deepseek_v4_test.py | Correct helper return typing for layer runner. |
| src/mobius/integrations/ort_genai/auto_export.py | Reuse emitted model type directly for compatibility metadata. |
| src/mobius/integrations/onnx_genai/inference_metadata.py | Clarify MTP proposer execution/doc distinctions for logits vs hidden. |
| src/mobius/integrations/gguf/_reuse.py | Shift to cheap identity checks during staging; hash once at final verify gate. |
| src/mobius/integrations/gguf/_reader_test.py | Add regression test for endian-aware unsupported-header reporting. |
| src/mobius/integrations/gguf/_quant_registry.py | Clarify lm_head quantization preservation policy docs. |
| src/mobius/integrations/gguf/_preflight.py | Document F64 passthrough alongside other float types. |
| src/mobius/integrations/gguf/_mtp.py | Improve MTP contract error message to reference exact metadata key/value. |
| src/mobius/integrations/gguf/_mtp_test.py | Update MTP error-message matching to new key-based wording. |
| src/mobius/integrations/gguf/_header.py | Improve unsupported GGUF version error to report both endian candidates. |
| src/mobius/integrations/gguf/_config_mapping.py | Refactor PLaMo2 attention width inference into shared helper. |
| src/mobius/integrations/gguf/_builder.py | Tighten TensorRole typing; reuse shared PLaMo2 width inference; clarify errors. |
| src/mobius/integrations/gguf/_builder_test.py | Add coverage for “hash once” reuse behavior + final publish-time verification. |
| src/mobius/integrations/gguf/_arch_registry_test.py | Remove obsolete verdict filtering in expected rows. |
| docs/cli_reference.md | Clarify graph-only vs runtime packaging behavior for MTP sidecars. |
| .agents/skills/attention-optimization/SKILL.md | Update kernel reference link to a stable GitHub URL. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+341
to
+349
| def _source_identity(path: Path) -> tuple[int, int, int, int, int]: | ||
| source_stat = path.stat() | ||
| return ( | ||
| source_stat.st_dev, | ||
| source_stat.st_ino, | ||
| source_stat.st_size, | ||
| source_stat.st_mtime_ns, | ||
| source_stat.st_ctime_ns, | ||
| ) |
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
maintainability, and performance findings left by the GGUF PR stack
gate, while retaining cheap identity checks around staging
Exact base:
2db9d33debdc254d879a51b14434c9a81c230f4fExact head:
4541ed2bc9d2ab4227484510b4a85b2d9113eb25Reconstructed original 17-item low-priority tranche
The persisted audit retained only the totals, so this list was reconstructed
from the live threads and current source. All but the already-fixed #596
comment are addressed in this PR.
TensorRole | NonetypingCurrent unresolved-thread disposition
This covers all 48 Copilot threads returned by the reproducible #600-#630
query. The one human #623 thread is excluded.
expert_dtypeis classified before early returntensor_items_raw()INT64_MAXsentinelValueError/issues/605Validation
lintrunner; full lint/format passed