[Tests]: Precommit Check for Spec-Dec Recipes - #1527
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughAdds a new pre-commit hook that scans launcher example YAMLs, extracts and resolves ChangesPre-commit validation system enhancements
sequenceDiagram
participant PreCommit as PreCommitHook
participant Scanner as LauncherScanner
participant Parser as YAMLParser
participant Resolver as PathResolver
participant FS as Filesystem
participant Loader as RecipeLoader
PreCommit->>Scanner: select tools/launcher/examples/*.yaml (or all when hook changed)
Scanner->>Parser: safe_load each YAML pipeline/tasks
Parser->>Resolver: extract --config and .chat_template paths
Resolver->>FS: resolve repo/submodule paths
FS->>Loader: call modelopt.recipe.loader.load_recipe for modelopt_recipes/* configs
Loader-->>PreCommit: return validation errors or success
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1527 +/- ##
==========================================
- Coverage 67.73% 67.65% -0.08%
==========================================
Files 511 511
Lines 56169 56256 +87
==========================================
+ Hits 38044 38060 +16
- Misses 18125 18196 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
A bit confused by the scope: is the change for general launcher yaml check or for spec-dec specific recipes check? |
Both. It adds checks for spec-dec recipes and all yamls that appears in the general launcher. |
| entry: python tools/precommit/check_launcher_yaml.py | ||
| language: system | ||
| files: ^(tools/launcher/examples/.*\.yaml|modelopt_recipes/.*\.yaml|tools/precommit/check_launcher_yaml\.py)$ | ||
| pass_filenames: false |
There was a problem hiding this comment.
hmm, so this behaviour is not expected:
It seems the precommit check script gets activated once there are modifications to any file in the above pattern, but then it will scan all the files even for files that have no modifications.
We should scope to only modified files.
There was a problem hiding this comment.
Fixed. Now this hook only scan the changed yaml files under tools/launcher.
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
b9296fc to
c306d15
Compare
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/precommit/check_launcher_yaml.py (1)
163-164: 💤 Low valueVerify performance impact of full re-scan on script edits.
When this script is modified, all launcher YAMLs are re-scanned. If the
examples/directory grows large, this could slow down commits that touch this file. Consider whether this safety mechanism's value justifies the performance trade-off.For now, this is a reasonable design—edits to validation logic should exercise the full test surface—but worth monitoring if the launcher YAML set grows significantly.
🤖 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 `@tools/precommit/check_launcher_yaml.py` around lines 163 - 164, The current guard (if path == this_file: return _all_launcher_yamls()) forces a full re-scan of all launcher YAMLs when this script is edited; measure the commit-time cost by timing _all_launcher_yamls() on a large examples/ tree and, if expensive, implement a faster alternative: either add an opt-in config/env flag (e.g., SKIP_FULL_RESCAN) to short-circuit the full scan for local edits, or replace the unconditional full scan with an incremental selection using git-changed-file logic to compute only affected launcher files, and ensure the change is wired into the same branch where path, this_file, and _all_launcher_yamls() are referenced so behavior is preserved when desired.
🤖 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.
Nitpick comments:
In `@tools/precommit/check_launcher_yaml.py`:
- Around line 163-164: The current guard (if path == this_file: return
_all_launcher_yamls()) forces a full re-scan of all launcher YAMLs when this
script is edited; measure the commit-time cost by timing _all_launcher_yamls()
on a large examples/ tree and, if expensive, implement a faster alternative:
either add an opt-in config/env flag (e.g., SKIP_FULL_RESCAN) to short-circuit
the full scan for local edits, or replace the unconditional full scan with an
incremental selection using git-changed-file logic to compute only affected
launcher files, and ensure the change is wired into the same branch where path,
this_file, and _all_launcher_yamls() are referenced so behavior is preserved
when desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b604c63a-0d87-4d16-b317-abc543e0b365
📒 Files selected for processing (2)
.pre-commit-config.yamltools/precommit/check_launcher_yaml.py
|
2 participants
What does this PR do?
Type of change: new tests / tooling
Adds pre-commit validation for speculative-decoding recipes (the existing
check-modelopt-recipeshook only ran on PTQ) and for launcher YAML references into the recipe library.tools/precommit/check_modelopt_recipes.py: acceptspeculative_eagle/speculative_dflash/speculative_medusain addition toptq, so per-model spec-dec recipes (e.g.modelopt_recipes/models/Qwen3-8B/dflash.yaml) get full Pydantic validation viaload_recipe()at commit time.tools/precommit/check_launcher_yaml.py(new): scans everytools/launcher/examples/**/*.yamlfor--config <path>anddata.chat_template=<path>references, verifies the resolved files exist, and runsload_recipe()on any path undermodelopt_recipes/. Skips<<global_vars.x>>interpolation.pass_filenames: falseso recipe-side edits also re-validate all launcher references.Usage
Testing
Smoke-tested both hooks manually:
dflash_block_size: not_an_int--configpathdata.chat_templatepathBefore your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Motivated by the per-model recipe migration in #TBD — without these hooks, broken
--configpaths and recipe schema typos surface only at CI or runtime.Summary by CodeRabbit