Normalize .yml to .yaml in modelopt_recipes#1260
Open
shengliangxu wants to merge 5 commits intomainfrom
Open
Conversation
both file extensions are valid, but for files that are completely under our control, might just normalize it to a single extension: .yaml. The .yaml extension seems to be preferred by the yaml spec. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a new local pre-commit hook and accompanying script to rename Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/precommit/normalize_yaml_ext.py`:
- Around line 35-38: The rename currently does not guard against destination
collisions and may silently overwrite existing files (os.rename(path,
new_path)); before calling os.rename in the block that handles .yml files (the
variables path, new_path and the renamed list), add an explicit check like if
new_path.exists(): handle it (raise an error or skip and log) to prevent
overwriting on POSIX and to ensure consistent cross-platform behavior; only call
os.rename(path, new_path) and append to renamed if the destination does not
already exist.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8bd9276-b5d7-468e-962f-4cc5f125fc6c
📒 Files selected for processing (8)
.pre-commit-config.yamlmodelopt_recipes/general/ptq/fp8_default-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_default-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_experts_only-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_mlp_only-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_omlp_only-fp8_kv.yamltests/unit/recipe/test_loader.pytools/precommit/normalize_yaml_ext.py
kevalmorabia97
approved these changes
Apr 14, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1260 +/- ##
==========================================
- Coverage 76.90% 72.15% -4.75%
==========================================
Files 350 350
Lines 40524 40524
==========================================
- Hits 31166 29242 -1924
- Misses 9358 11282 +1924
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/precommit/normalize_yaml_ext.py (1)
34-57: Avoid partial renames when collisions exist.Current flow can rename some files and still fail due to collisions, leaving a mixed working tree state. Prefer validating all candidates first, then applying renames only if there are no collisions.
Proposed refactor (all-or-nothing rename pass)
def main() -> int: """Rename .yml files to .yaml, exit 1 if any were renamed.""" - renamed: list[tuple[Path, Path]] = [] + planned: list[tuple[Path, Path]] = [] collisions: list[tuple[Path, Path]] = [] for f in sys.argv[1:]: path = Path(f) if path.suffix == ".yml" and path.is_file(): new_path = path.with_suffix(".yaml") if new_path.exists(): collisions.append((path, new_path)) continue - os.rename(path, new_path) - renamed.append((path, new_path)) + planned.append((path, new_path)) if collisions: for old, new in collisions: print(f"ERROR: Cannot rename {old} -> {new} (destination already exists)") return 1 + renamed: list[tuple[Path, Path]] = [] + for old, new in planned: + os.rename(old, new) + renamed.append((old, new)) + if renamed: for old, new in renamed: print(f"Renamed: {old} -> {new}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/precommit/normalize_yaml_ext.py` around lines 34 - 57, The current loop in normalize_yaml_ext.py performs renames as it iterates which can leave a partially-renamed working tree if collisions are later detected; change the logic in the main block that iterates over sys.argv[1:] to first collect all candidate renames (map Path -> new_path) and detect collisions/new_path existence using the same checks used for collisions, and only if collisions is empty perform all os.rename operations (populating renamed afterwards); keep the existing prints and return codes but ensure no os.rename is called until after the preflight collision check so the rename is all-or-nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/precommit/normalize_yaml_ext.py`:
- Around line 34-57: The current loop in normalize_yaml_ext.py performs renames
as it iterates which can leave a partially-renamed working tree if collisions
are later detected; change the logic in the main block that iterates over
sys.argv[1:] to first collect all candidate renames (map Path -> new_path) and
detect collisions/new_path existence using the same checks used for collisions,
and only if collisions is empty perform all os.rename operations (populating
renamed afterwards); keep the existing prints and return codes but ensure no
os.rename is called until after the preflight collision check so the rename is
all-or-nothing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d218306f-b097-432d-84ed-72faa57a9280
📒 Files selected for processing (2)
modelopt_recipes/general/ptq/nvfp4_default-none_kv_gptq.yamltools/precommit/normalize_yaml_ext.py
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
2 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.
What does this PR do?
Type of change: Chore
Standardize YAML file extensions in
modelopt_recipes/to.yamlforconsistency. The existing recipes used a mix of
.yml(PTQ recipes) and.yaml(speculative decoding, model-specific recipes).Changes
Renamed files:
modelopt_recipes/general/ptq/fp8_default-fp8_kv.yml→.yamlmodelopt_recipes/general/ptq/nvfp4_default-fp8_kv.yml→.yamlmodelopt_recipes/general/ptq/nvfp4_experts_only-fp8_kv.yml→.yamlmodelopt_recipes/general/ptq/nvfp4_mlp_only-fp8_kv.yml→.yamlmodelopt_recipes/general/ptq/nvfp4_omlp_only-fp8_kv.yml→.yamlNew pre-commit hook:
normalize-yaml-exttools/precommit/normalize_yaml_ext.py— auto-renames.ymlto.yamlfor any staged file under
modelopt_recipes/. Runs before recipevalidation so future contributions are caught automatically.
Updated references:
tests/unit/recipe/test_loader.py— built-in recipe paths updated to.yamlNote:
load_recipe()andload_config()probe both.ymland.yamlsuffixes, so callers using paths without extensions (e.g.,
load_recipe("general/ptq/fp8_default-fp8_kv")) are unaffected.Testing
Existing recipe loader tests pass with updated paths.
Summary by CodeRabbit