Add GLM DSA package contract validation - #1032
Merged
Merged
Conversation
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe changes add GLM-DSA GGUF cache-shape handling, a contract validator, generation-policy repair and preflight support, two CLI commands, and documentation for runtime and manifest requirements. ChangesGLM-DSA support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PackageCLI
participant ArtifactCollector
participant GGUFParser
participant ContractValidator
participant ManifestRepairer
Operator->>PackageCLI: validate or repair package
PackageCLI->>ArtifactCollector: collect GGUF artifacts
ArtifactCollector->>GGUFParser: parse metadata and tensor names
GGUFParser->>ContractValidator: provide artifact data
ContractValidator-->>PackageCLI: return contract report
PackageCLI->>ManifestRepairer: repair generation policy when requested
ManifestRepairer-->>PackageCLI: write and revalidate manifest
PackageCLI-->>Operator: print JSON result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/LAYER_PACKAGE_REPOS.md (1)
1-1: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the experimental policy docs with the model contract
generation.policy.experimental.moe_weighted_downandmoe_merged_shared_gate_upare shown as manifest fields in both docs, butPackageGenerationExperimentalPolicyonly modelsselected_row_flash, so the extra keys are currently ignored by serde and never validated. Either add them tocrates/skippy-model-package/src/glm_dsa_contract.rsandcrates/skippy-model-package/src/preflight.rs, or mark them as aspirational / not yet supported in the docs.🤖 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 `@docs/LAYER_PACKAGE_REPOS.md` at line 1, Align the documentation with the supported model contract by marking generation.policy.experimental.moe_weighted_down and moe_merged_shared_gate_up as aspirational or not yet supported wherever they appear in the package repository docs. Keep selected_row_flash documented as the currently modeled field, and do not present the unsupported keys as validated manifest fields.
🤖 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 `@crates/model-artifact/src/gguf.rs`:
- Line 237: Extract the self-contained GgufKvCacheQuant KV-cache derivation
logic and its associated tests from gguf.rs into a new kv_cache module,
preserving its public API and behavior. Update imports and references so
existing callers continue to work, and ensure gguf.rs is reduced below 1,000
lines.
In `@crates/skippy-model-package/README.md`:
- Around line 49-50: Update the README command to use the
validate-glm-dsa-contract subcommand with the existing model-package argument.
Revise the write-package guidance to state that it writes only
speculative_decoding, while generation.policy and generation.thresholds are
added by repair-glm-dsa-generation-policy.
- Around line 67-82: Revise the README generation-handling section to clarify
that write-package only populates generation.speculative_decoding, while
generation.policy and generation.thresholds are supplied by
repair-glm-dsa-generation-policy. Preserve the guidance on versioned policy
profiles and evidence-based defaults without implying that write-package alone
completes the GLM-DSA contract.
In `@crates/skippy-model-package/src/glm_dsa_contract.rs`:
- Line 1: Extract the duplicated GLM-DSA policy/threshold constants and
generation manifest structs into one shared module, exposing them as pub(crate).
Update glm_dsa_contract, preflight, and glm_dsa_generation_policy—including
expected_policy() and expected_thresholds()—to import and use those shared
definitions, removing their local declarations.
- Around line 17-37: Add cross-field validation to validate_metadata after the
individual REQUIRED_U32_METADATA checks: require
glm-dsa.attention.indexer.key_length to be greater than
glm-dsa.rope.dimension_count, and reject metadata when glm-dsa.expert_used_count
exceeds glm-dsa.expert_count. Use the existing validation error mechanism and
metadata lookup conventions, preserving the current positive-value checks.
- Around line 54-65: Update validate_decoder_layer to explicitly reject any
unsplit blk.N.attn_kv_b.weight tensor, including when split attn_k_b.weight and
attn_v_b.weight tensors are present. Preserve the existing BASE_LAYER_TENSORS
requirements while enforcing that split and unsplit KV-B layouts cannot coexist.
In `@crates/skippy-model-package/src/main.rs`:
- Around line 88-100: Update the ValidateGlmDsaContract handler to retain
printing the validation report, then return a non-zero error when report.valid
is false, matching the validation failure behavior used by
RepairGlmDsaGenerationPolicy. Keep successful validation returning Ok(()) and
preserve the existing JSON output.
---
Outside diff comments:
In `@docs/LAYER_PACKAGE_REPOS.md`:
- Line 1: Align the documentation with the supported model contract by marking
generation.policy.experimental.moe_weighted_down and moe_merged_shared_gate_up
as aspirational or not yet supported wherever they appear in the package
repository docs. Keep selected_row_flash documented as the currently modeled
field, and do not present the unsupported keys as validated manifest fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21a1f1c4-30dd-41a7-b2ef-a8008597a653
📒 Files selected for processing (10)
crates/model-artifact/src/gguf.rscrates/skippy-model-package/README.mdcrates/skippy-model-package/src/cli.rscrates/skippy-model-package/src/glm_dsa_contract.rscrates/skippy-model-package/src/glm_dsa_generation_policy.rscrates/skippy-model-package/src/main.rscrates/skippy-model-package/src/preflight.rsdocs/LAYER_PACKAGE_REPOS.mddocs/specs/GLM_DSA_RUNTIME_CONTRACT.mddocs/specs/layer-package-repos.md
michaelneale
approved these changes
Jul 21, 2026
michaelneale
left a comment
michaelneale
left a comment
Collaborator
There was a problem hiding this comment.
Approving. Machine-checkable GLM-DSA package contract validation - the right "fail before runtime, not during lab" instinct, and it's cleanly scoped:
- New validation logic is self-contained in
glm_dsa_contract.rs/glm_dsa_generation_policy.rs; preflight extension is additive. - The one shared-surface edit (
model-artifact/src/gguf.rsaddingkv_lora_ranktoGgufCompactMeta) is safe: that struct is in-memory sizing only (derive(Clone, Debug, Default)), not a gossip/wire type, so no mixed-version mesh impact. - The KV-cache sizing change is gated behind
architecture == "glm-dsa", so it can't perturb sizing for any other model family. - CI green incl. clippy, and it carries a real test.
One sequencing note (not a blocker for this PR): it edits skippy-model-package/src/preflight.rs, which #1026 also rewrites - so this should land before #1026 to avoid a conflict.
michaelneale
added a commit
that referenced
this pull request
Jul 22, 2026
…ngram * origin/main: Add GLM DSA correctness trace reporting (#1033) Add GLM DSA package contract validation (#1032) Add direct quant layer package tooling (#1031) Lock split topology placement (#1050) fix nightly stability Qwen thinking ui(data-mode): migrate legacy harness storage to v2 (#1040) fix: Windows Vulkan runtime dependencies (#1046) test: validate MI300 runtime selection and placement (#1045) fix: support ROCm RDNA APU kernels safely (#1044) fix: ROCm GPU enumeration (#1039) Document public packaging installation channels Fix Windows autoupdate import (#1030) Update llama.cpp upstream pin
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
3 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.
Summary
Why
GLM-DSA layer packages need a machine-checkable contract before they are loaded in Skippy. This makes malformed IndexShare schedules, missing Full/Shared metadata, and missing generation policy thresholds fail before runtime rather than during lab execution.
Validation
cargo test -p skippy-model-packagecargo check -p skippy-model-packagecargo clippy -p skippy-model-package --all-targets -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation