lablup / lablup/mlxcel

test(surgery): cover ModelDims::from_config and validate_ids directly

Open Beginner friendly
#1,697 0 comments 0 reactions 0 assignees View on GitHub
priority:low status:ready type:test
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.