test(surgery): cover ModelDims::from_config and validate_ids directly
- Dominant language
- Rust
- Stars
- 467
- Forks
- 54
- Avg merge
- 4h 25m
- Merged PRs (30d)
- 310
Description
## Summary
Add direct tests for `ModelDims::from_config` and `validate_ids` in the surgery prune op. Both are pure `serde_json::Value -> Result` helpers with enumerated branches and six distinct error messages, and today only the happy path runs through them.
## Background
The per-granularity prune tests pass a single valid config through these helpers, so the fallback branches and every error message are untested. The crate builds without a platform backend, so table-driven JSON fixtures cover this with no I/O and no MLX.
## Proposed Solution
Table-driven JSON fixtures over the branches and error messages of both helpers. Optionally hoist the duplicated `num_heads == 0` guard so the second copy becomes redundant.
## Implementation Notes
- `src/lib/mlxcel-surgery/src/ops/prune/model_dims.rs:49-113` (`from_config`) and `:146-188` (`validate_ids`): no `#[cfg(test)]` in the file; the only caller is `ops/prune/mod.rs:213`.
- Branches worth pinning (confirmed in source): the VLM `text_config` fallback (`:53`), `num_key_value_heads` defaulting to `num_heads` (`:61`), `head_dim` derived from `hidden_size / num_heads` (`:66`), and each error message.
- Sibling tests (`ops/prune/tests/layer.rs`, `attention_head.rs`, `mlp_channel.rs`) exercise only the happy path.
- Secondary: the `num_heads == 0` guard appears twice (inside the `head_dim` derivation and again unconditionally); hoisting it above makes the second redundant.
## Acceptance Criteria
- [ ] Every branch and error message of both helpers has a direct test.
---
## Original Suggestion
### Title: test(surgery): cover ModelDims::from_config and validate_ids directly
`ModelDims::from_config` and `validate_ids` in the surgery prune op are pure `serde_json::Value → Result` helpers with clearly enumerated branches and six distinct error messages — and no direct tests; the per-granularity tests only pass a single valid config through them.
## Evidence
- `src/lib/mlxcel-surgery/src/ops/prune/model_dims.rs:49-113` and `:146-188` — no `#[cfg(test)]` in the file; only caller is `ops/prune/mod.rs:213`
- Branches worth pinning: the VLM `text_config` fallback (`:51-57`), `num_key_value_heads` defaulting to `num_heads` (`:61-64`), `head_dim` derived from `hidden_size / num_heads` (`:66-86`), and each error message
- Sibling tests (`ops/prune/tests/layer.rs`, `attention_head.rs`, `mlp_channel.rs`) exercise only the happy path
Secondary observation for the same PR: the `num_heads == 0` guard is written twice (`:74-78` inside the head_dim derivation, and unconditionally at `:90-94`); hoisting it above makes the second redundant.
## Suggested fix
Table-driven JSON fixtures over the branches; no I/O, no MLX — the crate builds without a platform backend. Optionally hoist the duplicated guard.
## Acceptance criteria
- [ ] Every branch and error message of both helpers has a direct test
Contributor guide
Research direction
Start with src/lib/mlxcel-surgery/src/ops/prune/model_dims.rs, reading ModelDims::from_config and validate_ids at the cited ranges. Check the existing happy-path coverage in ops/prune/tests/layer.rs, attention_head.rs, and mlp_channel.rs, then run the crate tests without a platform backend. Done means direct table-driven tests cover every listed branch and error message.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- machine-learning, testing
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 86/100