Establish scoped Puzzletron v2 quality checks - #2145
Conversation
Check pull request changes without inheriting repository-wide lint debt, and compare full documentation builds against the established feature-branch diagnostic baseline. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Fix the documentation warnings directly and scope lint enforcement to files changed against the Puzzletron v2 target. This preserves strict checks without inheriting repository-wide formatting debt. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
|
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. |
📝 WalkthroughWalkthroughThe pull request updates changed-file CI checks, Sphinx autodoc behavior and references, Puzzletron pruning validation, source formatting, and automodel unit-test maintenance. ChangesQuality and Puzzletron maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modelopt/torch/puzzletron/pruning/dynamic_block_prune.py (1)
134-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace required-parameter assertions with explicit exceptions.
Python removes these assertions with
-O. Missing metadata can then reachFFNRemovalSpecor fail later during attention mask creation. RaiseValueErrorfor missing metadata.🤖 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 `@modelopt/torch/puzzletron/pruning/dynamic_block_prune.py` at line 134, Replace the assert validating down_proj_name with an explicit ValueError when the required metadata is missing, ensuring validation remains active under optimized Python execution before constructing FFNRemovalSpec or creating attention masks.Source: Coding guidelines
🤖 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.
Inline comments:
In @.github/workflows/code_quality.yml:
- Around line 22-27: Add persist-credentials: false to the with configuration of
both actions/checkout@v6 steps, preserving fetch-depth: 0 in the pull_request
checkout and leaving the existing event conditions unchanged.
---
Nitpick comments:
In `@modelopt/torch/puzzletron/pruning/dynamic_block_prune.py`:
- Line 134: Replace the assert validating down_proj_name with an explicit
ValueError when the required metadata is missing, ensuring validation remains
active under optimized Python execution before constructing FFNRemovalSpec or
creating attention masks.
🪄 Autofix
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: Enterprise
Run ID: f48bf9d5-9645-40e5-834e-2e96eb3022fe
📒 Files selected for processing (10)
.github/workflows/code_quality.ymldocs/source/conf.pydocs/source/guides/_basic_quantization.rstdocs/source/guides/_pytorch_quantization.rstdocs/source/guides/_quant_cfg.rstmodelopt/torch/puzzletron/anymodel/capabilities.pymodelopt/torch/puzzletron/mip/grid_budgeting.pymodelopt/torch/puzzletron/pruning/dynamic_block_prune.pynoxfile.pytests/unit/torch/puzzletron/test_automodel_local_kd_config.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/puzzletron_v2 #2145 +/- ##
=========================================================
+ Coverage 53.65% 62.09% +8.43%
=========================================================
Files 706 707 +1
Lines 91275 91347 +72
=========================================================
+ Hits 48978 56720 +7742
+ Misses 42297 34627 -7670
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:
|
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
What does this PR do?
Type of change: Bug fix and new tests
Pull requests targeting
feature/puzzletron_v2inherit repository-wide lint and documentation failures, which prevents unrelated changes from establishing a useful quality baseline. This PR introduces a scoped quality ratchet without claiming full branch-wide CI recovery.This change:
types-docutilsin the development lint dependencies so changed documentation configuration is type-checked deterministically in uv-created nox environments.ValueErrorfailures.The test changes are explicit:
Path.resolve().The public Puzzletron API and configuration surface are unchanged. Invalid internal pruning metadata now fails earlier with
ValueError, andtypes-docutilsremains limited to the development lint extra.Testing
The Code Quality workflow covers the target-specific changed-file pre-commit and mypy route, including the pinned documentation stubs and checkout hardening. The Documentation workflow covers the warning-free Sphinx build. The dedicated Puzzletron v2 CPU workflow covers the adjusted local-KD module and the new pruning-validation cases.
The broader Example and GPU required checks are not made green by this PR. A separate follow-up will address inherited TRL compatibility failures in the
gpt-ossandllm_distillexamples, retire or replace stale v1 Puzzletron GPU coverage that still calls the deletedentrypointAPI, and re-evaluate the external Nemotron import, DeepSpeed numerical, and dataset-download failures.Additional Information
This PR is intentionally limited to the quality ratchet and focused Puzzletron validation changes. Full branch-wide Example and GPU CI recovery is follow-up work.