chore(lint): modernize typing (PEP 604/585) and enable UP032#1537
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughModernizes typing to use TypeAlias and ChangesPython Syntax Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
- Replace `X = Union[A, B] # noqa: UP007` with `X: TypeAlias = A | B` for the six module-level type aliases (ModelLike, Criterion, NodeTarget, CalibrationDataType, Hparam.Importance/ActiveSlice). TypeAlias is needed so mypy still treats them as aliases under PEP 604 syntax. - Modernize forward-ref unions in onnx/quantization/autotune to full-string forward refs (e.g. `"RegionPattern | None"`). - Update docstring type tags in examples. Intentionally left: - tools/launcher/slurm_config.py: documented `# ruff: noqa: UP045` (nemo_run CLI parser can't introspect PEP 604 optional annotations). - modelopt/torch/puzzletron/*: skipped. Subtree has its UP lint rules disabled (per-file-ignore set to "UP"), and converting Optional[X] to X | None there silently breaks runtime introspection that uses `get_origin(tp) is Union` (see block_config._get_dataclass_type). Will revisit when puzzletron's lint carve-outs are narrowed. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
UP045 already had no violations. UP008 stays scoped to puzzletron (via the broader "UP" carve-out — puzzletron files were intentionally not touched in this branch; see the typing-modernization commit for the rationale). Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
5961c16 to
397bb62
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1537 +/- ##
==========================================
- Coverage 76.83% 76.80% -0.03%
==========================================
Files 477 477
Lines 51954 51957 +3
==========================================
- Hits 39917 39904 -13
- Misses 12037 12053 +16
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:
|
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
🤖 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 `@examples/puzzletron/evaluation/hf_deployable_anymodel.py`:
- Line 103: Replace the assert-based validation "assert task in SUPPORTED_TASKS"
with an explicit runtime check that raises a ValueError when the provided task
is not in SUPPORTED_TASKS; locate the validation that uses the task variable
(and SUPPORTED_TASKS) in hf_deployable_anymodel.py and change it to: if task not
in SUPPORTED_TASKS: raise ValueError(f"Unsupported task: {task}") so the check
remains active in optimized runs and clearly signals invalid external input.
🪄 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: Enterprise
Run ID: e744cc83-6c5e-4e96-9402-977ebe30e16e
📒 Files selected for processing (17)
examples/puzzletron/evaluation/hf_deployable_anymodel.pyexamples/speculative_decoding/collect_hidden_states/send_conversations_for_hiddens.pyexamples/speculative_decoding/scripts/send_conversation_vllm.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/region_pattern.pymodelopt/onnx/quantization/calib_utils.pymodelopt/torch/distill/config.pymodelopt/torch/export/plugins/mcore_custom.pymodelopt/torch/export/plugins/megatron_importer.pymodelopt/torch/opt/hparam.pymodelopt/torch/utils/graph.pymodelopt/torch/utils/network.pymodelopt/torch/utils/plugins/megatron_generate.pymodelopt/torch/utils/plugins/transformers_dataset.pypyproject.tomltests/examples/speculative_decoding/test_eagle.pytools/launcher/common/query.py
💤 Files with no reviewable changes (1)
- pyproject.toml
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Mechanical lint cleanup: PEP 604/585 modernization + f-string conversion. The complexity gate fired on directory count, but no new subsystem/DSL/loader is introduced — the design-review protocol doesn't apply. Spot-checked the non-trivial conversions: the : TypeAlias annotations are correct (and necessary so mypy keeps treating these as aliases under PEP 604), forward-ref unions are kept as full strings where the referent is TYPE_CHECKING-only, and the f-string conversions preserve formatting specs (including the manual line-wrap in megatron_generate.py). The PR body explicitly calls out the puzzletron carve-out and the get_origin is typing.Union foot-gun, which is the right reasoning. No tests needed for a pure style refactor.
Complex PR: spans 13 directories (≥ 5). Looping in a human for approval.
What does this PR do?
Type of change: chore / refactor (no behavior change)
Two small lint-cleanup commits:
1.
chore(typing): modernize Union/Optional/List to PEP 604 / 585 syntax(8 files)X = Union[A, B] # noqa: UP007withX: TypeAlias = A | Bfor the six module-level type aliases (ModelLike,Criterion,NodeTarget,CalibrationDataType,Hparam.Importance/ActiveSlice). TheTypeAliasannotation is required so mypy continues to treat them as aliases under PEP 604.modelopt/onnx/quantization/autotune/to full-string forward refs (e.g."RegionPattern | None").examples/puzzletron/evaluation/hf_deployable_anymodel.py.2.
chore(lint): remove UP032 ignore and convert .format() to f-strings(10 files)UP032fromextend-ignoreinpyproject.toml."...".format(...)calls to f-strings across export plugins, examples, tests, and tools. One conversion inmodelopt/torch/utils/plugins/megatron_generate.pywas wrapped manually to stay under the 100-char limit.Intentionally left as-is:
tools/launcher/slurm_config.pykeeps its# ruff: noqa: UP045— nemo_run's CLI parser can't introspect PEP 604 optional annotations.modelopt/torch/puzzletron/*is not touched. The subtree disables ruff'sUPfamily entirely (per-file-ignore"UP") while migration is in progress, and convertingOptional[X]toX | Nonethere would silently break runtime introspection inblock_config._get_dataclass_typethat usesget_origin(tp) is typing.Union(PEP 604 unions returntypes.UnionTypefromget_origin, nottyping.Union). Best revisited when puzzletron's lint carve-out is narrowed.UP038(isinstance(x, (int, float))→isinstance(x, int | float)) — ruff has officially deprecated this rule; PEP 604 in isinstance is slightly slower and misleads readers about PEP 695 /Optional. Ignore kept.Usage
No user-facing API changes.
Testing
main: 37 unrelated pre-existing findings (W291/W293/E501/RUF005/PLR1704); zero new findings introduced by this PR.Before your PR is "Ready for review"
typing.Unioninstance totypes.UnionType. Downstream code introspecting viaget_origin(...) is typing.Unionon these aliases would break, but no in-repo caller does this on them. (The introspection inmodelopt/torch/puzzletron/block_config.pyoperates on user-supplied dataclass field types, none of which are these aliases.)CONTRIBUTING.md: N/ASummary by CodeRabbit
Style
Chores