fix(mlx-ci): align smoke step count assertion#5622
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Aligns the real MLX smoke test assertion and docs with the current 30-step training configuration introduced in #5537.
Changes:
- Replaces hardcoded
7in the step count assertion withconfig.max_steps. - Updates docstring to reflect 30 steps, 180 total sequences, and the teacher-forced completion-loss gate.
- Updates LoRA target description and the MLX CI workflow comment to match current behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/studio/run_real_mlx_smoke.py | Fixes step count assertion and refreshes docstring for LoRA targets, step count, and completion gating. |
| .github/workflows/mlx-ci.yml | Updates CI workflow comment from 7 to 30 deterministic LoRA steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the run_real_mlx_smoke.py test script by expanding LoRA target modules to include gate, up, and down layers, and increasing the training duration from 7 to 30 steps. The validation logic was also updated to support regression telemetry. A review comment suggests improving the assertion error message to display the actual count of logged steps rather than the entire list of losses for better clarity.
| if k in train_result | ||
| } | ||
| assert len(losses_per_step) == 7, f"expected 7 logged steps, got {losses_per_step}" | ||
| assert ( |
There was a problem hiding this comment.
The assertion error message is confusing as it prints the entire list of losses instead of the count. It is better to use dynamic configuration values like config.max_steps for clarity, ensuring the message accurately reflects the expected state.
assert len(losses_per_step) == config.max_steps, f"expected {config.max_steps} logged steps, got {len(losses_per_step)}"References
- User-facing warning messages should be dynamically generated to include the specific configuration values they refer to, rather than using hardcoded examples.
Summary
config.max_stepsinstead of the stale hardcoded7.Why
The smoke was changed to train for 30 steps in #5537, but the post-train callback assertion still expected 7 logged steps. Real MLX training completes and converges, then CI fails on the stale harness assertion.