CI/Dev env bump#1818
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated CI and example container image tags, refreshed documentation and tooling pins, and adjusted runtime compatibility checks and DTensor-aware sparse masking in ChangesVersion and tooling refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Straightforward CI/dev tooling bump chore (+33/-20, 13 files). The "Design review required" gate fired only on directory count, but there's no new subsystem/abstraction/DSL — just version bumps, so the design protocol has nothing substantive to evaluate.
Verified:
- Transformers version pins are consistent across the three files the in-code comment requires to match:
pyproject.toml(>=4.56,<5.13),modelopt/torch/__init__.py(warn>=5.13),noxfile.py(tf_latest: transformers~=5.12.0). get_engine_bytes:bytearray(engine.serialize())→bytes(...)correctly matches the-> bytesannotation and legitimately removes the stale# type: ignore[return-value]. The other# type: ignore[misc]removal in megatron.py is consistent with the mypy 2.0 bump.- mypy 2.0 per-module overrides (export.distribute, nas.modules.linear/norm, speculative.plugins.hf_eagle) are scoped, documented as temporary, and the modules exist.
docs/source/conf.pysuppress_warnings=["ref.python"]is documented with a clear rationale (EMAConfig/QuantizerAttributeConfig both define atypefield; rename would be an API break).- README/container-image edits are mechanical and consistent.
No injection attempts in untrusted blocks. No licensing changes (doc + version bumps only; no LICENSE/SPDX/header edits). Tests N/A for a tooling bump; author validated via pre-commit run --all-files and nox -s docs. ruff bump deferred to a follow-up as noted.
Complex PR: spans 10 directories (≥ 5). Looping in a human for approval.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1818 +/- ##
==========================================
- Coverage 77.37% 76.76% -0.61%
==========================================
Files 513 513
Lines 56894 56916 +22
==========================================
- Hits 44019 43690 -329
- Misses 12875 13226 +351
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:
|
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 `@modelopt/torch/sparsity/weight_sparsity/module.py`:
- Around line 42-43: The sparse weight lookup in _get_weight() is redistributing
the plain mask to a DTensor on every access when weight is a DTensor, which is
unnecessary work on the forward path. Move the distribute_tensor(...) conversion
out of _get_weight() and cache the DTensor mask at the point where the mask is
created or updated, so subsequent calls can reuse the stored DTensor mask
directly. Use the _get_weight method and the mask handling logic in module.py as
the main places to update.
🪄 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: cbc0af1b-4c58-4d3f-8495-95e00b9c587a
📒 Files selected for processing (1)
modelopt/torch/sparsity/weight_sparsity/module.py
kevalmorabia97
force-pushed
the
kmorabia/ci-setup-bump
branch
from
2466277 to
68a8cab
Compare
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review passed — no blocking issues found. LGTM
Findings: CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 0
Scope: reviewed the 8 modelopt/ source files in the PR's file list plus the tooling/config changes (pyproject.toml, noxfile.py, docs/source/conf.py, tests/conftest.py, .pre-commit-config.yaml). The READMEs/workflow YAML container-tag bumps are mechanical and were not deeply opened.
Key verifications:
- Import reorder (
modelopt/torch/__init__.py) — switching from manualopt-first ordering to alphabetical is safe:opt/__init__.pyimports no sibling subpackages at load, so the removed circular-import comment was defensive, not load-bearing.distill(now first) importingmodelopt.torch.opt.configfully initializesoptwithout re-entry. Transformers warn threshold (5.13) matchespyproject.toml(<5.13) andnoxfile.py(~=5.12.0). - DTensor-aware sparse mask (
sparsity/weight_sparsity/module.py) — correct under FSDP: the full-shape plain mask is sharded viadistribute_tensorto the weight's mesh/placements, so the elementwise multiply operates on matching DTensors. Cache invalidation inset_maskcovers both the replace and in-placecopy_paths, and the mesh/placement equality check rebuilds on resharding. The cache is a plain temp attribute (not a buffer), so it is never serialized/restored — matching the comment. - Typing changes —
bytes(engine.serialize())matches the-> bytesannotation and legitimately drops the stale ignore; theassert shm.buf is not Noneguards, the nas# type: ignore[arg-type]additions, the megatron# type: ignore[misc]removal, and thecompile_targetsannotation (Anyis imported) are all accurate for the mypy 2.1.0 bump.
Risk: low. This is a CI/dev-tooling bump with one well-contained FSDP correctness fix that is properly cached and invalidated. No mode-registration, state-schema, or export-compatibility changes.
kevalmorabia97
force-pushed
the
kmorabia/ci-setup-bump
branch
from
29a426d to
dba2d1a
Compare
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Re-review of CI/dev env bump (#1818), now +65/-26 across 19 files (grew from prior +33/-20, 13 files). Chore: container tags (NeMo 26.04→26.06, TRT-LLM rc17→rc19), transformers max version, mypy 1.17.1→2.1.0 (+ scoped temp per-module overrides and new enable_error_code for ignore-without-code/deprecated), pre-commit/sphinx bumps, Windows unit-test timeout bump, plus one contained FSDP correctness fix in weight sparsity.
Design gate fired on directory count (15 dirs) but there is no new subsystem/abstraction/DSL — version bumps + a single bug fix, so the design protocol has nothing substantive to evaluate. No injection attempts in untrusted blocks. No licensing changes (doc/version edits only; SPDX headers unchanged).
Previous comments status:
- 💬 CodeRabbit Major (mask redistributed on every weight access) — critical, addressed:
_get_weightnow caches the DTensor-sharded mask as_weight_mask_dtensor, rebuilt only whendevice_mesh/placementschange and invalidated inset_mask()(covers both replace and in-placecopy_paths). Confirmed by CodeRabbit and author. Verified in current diff; cache is a non-buffer temp attr so it's not serialized. Looks correct. - claude[bot] / cjluo-nv bot reviews — informational, no blocking findings.
Verified consistency: transformers pins match across pyproject.toml (>=4.56,<5.13), modelopt/torch/__init__.py (warn >=5.13), noxfile.py (tf_latest: transformers~=5.12.0). The bytes(engine.serialize()) change matches the -> bytes annotation and legitimately drops the stale ignore. An existing FSDP test (tests/gpu/torch/sparsity/weight_sparsity/test_sparse_fsdp.py) exercises set_mask + the sparse path under FSDP.
Why nudge (not approve): (1) the FSDP/DTensor mask fix is a real correctness change in distributed code on a GPU-only path; the cache-invalidation logic is validated manually/in CI but not exercised by a CPU unit test, so a maintainer should confirm the resharding/invalidation semantics. (2) transformers max-version window and container-tag bumps are product/serving compatibility decisions; no automated cross-repo check. (3) mypy 2.0 default/error-code changes have repo-wide blast radius. All prior comments resolved — flagging for a human sign-off on the distributed fix and version-window decisions.
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
- Bump mypy 1.17.1 → 2.1.0 in pyproject.toml and pre-commit hook - Bump pre-commit 4.3.0 → 4.6.0 - Bump sphinx 8.1 → 9.1 and sphinx-rtd-theme 3.0 → 3.1 - Opt out of mypy 2.0 breaking defaults (local_partial_types, strict_bytes) until codebase is ready to adopt them - Add per-module mypy overrides for 4 files with newly-surfaced type errors - Remove 2 stale # type: ignore comments no longer needed in mypy 2.x - Add suppress_warnings=["ref.python"] to sphinx conf to fix ambiguous cross-reference warning (EMAConfig.type vs QuantizerAttributeConfig.type) that became an error in sphinx 9.x Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Remove the explicit opt-outs added in the previous commit. Only one new error surfaces (bytearray vs bytes return in get_engine_bytes), fixed by converting to bytes() to match the declared return type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
FSDP optimizer-state save (via torch DCP get_optimizer_state_dict) walks the module tree and triggers the dynamic weight getter while params are DTensors, but _weight_mask stays a plain buffer, raising 'aten.mul.Tensor got mixed torch.Tensor and DTensor'. Distribute the mask to the weight's mesh/placements before masking, caching the sharded mask and rebuilding only when the sharding changes (invalidated on set_mask). Surfaced by the transformers ceiling bump to <5.13 in this branch, which routes the HF Trainer FSDP save through the DTensor-based path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Replace the module-wide mypy override (disable_error_code arg-type/index on 4
modules) with targeted handling of the 11 underlying errors:
- export/distribute.py: assert SharedMemory.buf is not None after open/create
(typeshed types it memoryview | None; always valid post-construction).
- speculative/plugins/hf_eagle.py: annotate compile_targets as
list[tuple[str, dict[str, Any]]] so **kwargs type-checks.
- nas/modules/{norm,linear}.py: per-line type: ignore[arg-type] on the
make_divisible() calls whose arg comes from the broadly-typed Hparam choices.
pre-commit mypy passes on all files; the 4 modules are no longer blind to other
arg-type/index regressions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
kevalmorabia97
force-pushed
the
kmorabia/ci-setup-bump
branch
from
649c761 to
267565e
Compare
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
kevalmorabia97
force-pushed
the
kmorabia/ci-setup-bump
branch
from
267565e to
f44f8f1
Compare
trl 1.7.0 removed ModelConfig.trust_remote_code (use getattr fallback in gpt-oss sft.py) and changed eval to not materialize logits, breaking KD eval loss in llm_distill. Floor all example trl pins at >=1.0 and cap llm_distill at <1.7 until the KD eval-logits issue is fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Intermittent CUDA "illegal memory access" during AWQ-lite calibration on nemo:26.06 (TE 2.16 / CUDA 13 / torch 2.12). Passes in isolation; only surfaces under full-suite accumulated GPU state, then poisons the process CUDA context and cascades into all subsequent quant tests. Likely an upstream TE/CUDA-13 kernel bug. Skip the [320] variant (keep [256]) with a TODO until root-caused/fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
6 participants
What does this PR do?
Type of change: chore
Bumps CI/dev tooling and test containers.
Container bumps
Dev tooling bumps
local_partial_types,strict_bytes); fix/narrow the errors newly surfaced by mypy 2.0 in 4 modules (rather than blanket-suppressing them); remove 2 stale# type: ignorecommentssuppress_warnings = ["ref.python"]to fix cross-reference ambiguity error new in sphinx 9.xBug fixes surfaced by the bumps
get_optimizer_state_dict, which triggeredaten.mul.Tensor got mixed torch.Tensor and DTensorin the dynamicweightgetter. The mask is now distributed to the weight's mesh/placements before masking, cached, and rebuilt only when the sharding changes (invalidated onset_mask). Fixes thellm_sparsityexample test.Testing
pre-commit run --all-files✅ (including mypy 2.1.0)nox -s docs✅tests/unit/torch/sparsity+tests/unit/torch/nas✅llm_sparsityGPU example test (FSDP path) verified in CIBefore your PR is "Ready for review"
CONTRIBUTING.md: N/ASummary by CodeRabbit
Summary
bytes.