Skip to content

CI follow-up: cache source-built wheels and extensions, bump CI containers, trim slow GPU tests - #2836

Open
kevalmorabia97 wants to merge 28 commits into
mainfrom
kmorabia/ci-gpu-followup
Open

kevalmorabia97 wants to merge 28 commits into
mainfrom
kmorabia/ci-gpu-followup

Conversation

@kevalmorabia97

@kevalmorabia97 kevalmorabia97 commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Type of change: CI / tests (follow-up to #2832)

  • gpu_megatron timeouts, root cause: Megatron's destroy_model_parallel() only drops its references to the NCCL groups, so each test's communicators stayed alive in the persistent workers: about 20 groups and 0.4 GiB of device memory per test (1221 groups and 24 GiB after 61 tests of test_megatron.py). Later tests got slower until they hit the 120 s timeout (the nightly cancellations), and smaller GPUs hit NCCL CUDA errors. megatron_worker_teardown now destroys every non-default group; test_megatron.py stays at 1 group and ~1-3 GiB and passes 131/131 in 4:54 on 2 GPUs (was 3 failures and slower).
  • Worker pool: a failing rank is now reported before its teardown runs. In gpu_megatron, teardown starts with dist.barrier(), which blocked against a still-running peer, so the parent never heard of the failure and the 30 s grace period could not start (it waited for the pytest timeout). New test fails without the fix. Also the remaining Fix post-merge unit CI and the gpu_megatron nightly timeout #2832 review nits (fixture-scope note, comments) and the sub-millisecond get_live_pool test moved to tests/unit.
  • Wheel cache for the gpu job: mamba_ssm, causal-conv1d and fast-hadamard-transform were compiled from sdist on every run (~10 min of the job, e.g. 4 + 6 min on 10-16). They are now pinned in noxfile.py and built into a wheelhouse that actions/cache keeps per container image (key includes the noxfile.py hash); a wheel is only built when the wheelhouse lacks it. A plain wheelhouse rather than pip's or uv's cache: pip does not cache wheels it builds from a local source tree (the C++20-patched sdist) or reliably from a git URL, its cache lives under HOME (/github/home in container jobs), and the GPU sessions use the container's own pip (venv_backend="none"), not uv. A directory of wheels keyed on the image and noxfile.py is explicit and is saved only once complete.
  • CUDA-extension cache never worked for the nightly: across 25 nightlies the gpu job never found its torch_extensions cache (205-350 s of extension compile each night; gpu_megatron hit about half the nights). actions/cache saves only when the job succeeds, and the nightly was failing or timing out every night, so main's cache scope was never populated (PR runs save into their own pull-request/N scope, which the nightly cannot read). The extension cache and the wheelhouse are now restored with actions/cache/restore and saved with actions/cache/save under always() (new .github/actions/save-extensions).
  • Container bumps: pytorch 26.05/26.07 -> 26.09. TensorRT-LLM stays at rc20: no later release runs the multimodal quickstart that test_qwen_vl uses (rc21-rc23 import MultimodalConfig, which tensorrt_llm.llmapi does not export yet; rc24+ import fla, which the image lacks). NeMo 26.08 is already the newest public image. vLLM stays at v0.30.0: v0.31.0 breaks the vLLM plugin (test_kimi_gemm_rs_ar_bypasses_enabled_fake_quantizers: gemm_rs_ar has no .apply; test_flashinfer_metadata_builder_patch_stashes_common_metadata: FlashInferMetadataBuilder.cache_config; test_tiny_llama_quantize times out) and needs plugin changes by its owners. The gpu_vllm job on v0.20.0 (legacy FusedMoE registration, vLLM < 0.24) is dropped; v0.30.0 is the only vLLM job, and its timeout goes from 15 to 20 min (its tests take ~10 min plus ~5 min to pull the image, so it timed out twice with all tests passing).
  • Extension cache key: keyed only on the C++/CUDA sources and their build scripts, so editing a Triton kernel no longer forces a rebuild (that rebuild pushed gpu_vllm past its 15 min limit). The key now also includes the job name: example and GPU jobs on the same image shared one key, and an example job that builds no extensions finished first and saved an empty cache. gpu and gpu_megatron then restored it as an exact hit, recompiled on every run (test_cuda_ext took 35-46 s in every gpu run of this PR), and could never save their own (Unable to reserve cache ... another job may be creating this cache). This also affects main. The save step now skips when no .so was built, so a run cancelled before building anything cannot claim the key with an empty cache. The tensorrt:26.05 images used by the ONNX/diffusers examples stay: torch-tensorrt (<2.13) needs libnvinfer.so.10, which newer images drop.
  • mamba_ssm / causal-conv1d on pytorch:26.09: their setup.py hardcodes -std=c++17 but the 26.09 torch headers need C++20 (ATen requires C++20), so the wheels are built with the standard patched to C++20 (checked to compile and import against torch 2.13 headers).
  • NVFP4 scale conversion without TensorRT-LLM's auto_deploy (removed in rc29, so the next bump would otherwise break): ModelOpt's NVFP4 dequantize/export paths (and gpu_trtllm collection) failed with ModuleNotFoundError. The cutlass-swizzled block-scale conversion is a tensor permutation, now implemented in ModelOpt as the inverse of swizzle_nvfp4_scales (same math as TensorRT-LLM's helper; bit-identical to rc20's on 6 shapes) with a CPU round-trip test.
  • conv3d implicit-GEMM JIT build: implicit_gemm_cuda.py passed -std=c++17, which fails against the 26.09 torch headers (165 GPU test failures); torch's load() now picks the standard.
  • FSDP2 + layerwise calibration bug: persistent_materialization patched FSDPParamGroup.unshard/reshard on the class, so while one layer was materialized every FSDP module skipped unshard; a full forward then ran on sharded DTensor params (test_layerwise_calibrate_fsdp2 failed on torch 2.14 with 'FSDPParam' object has no attribute '_unsharded_param'). Now patched only on the param groups inside the layer, and only when the layer itself is FSDP-wrapped (otherwise nothing was gathered for its FSDP submodules, so they must still unshard). CPU/gloo regression test fails without the fix; a second test covers a shared fully_shard([a, b]) group, nested contexts, a pre-existing instance override and a non-FSDP layer (fails without the group dedup or the FSDP-wrapped check).
  • EAGLE training crash (found by the new container): hf_eagle.py compiled _eagle_forward with mode="max-autotune", which enables inductor CUDA graphs; recording them intermittently fails (cudaStreamCaptureStatusInvalidated / "graph recording observed an input tensor deallocate"). It failed test_llama_eagle3 on 5 of the last 10 nightlies and on the pytorch:26.09 dispatch run. Now max-autotune-no-cudagraphs (same autotuning, no graphs). Training-speed impact not measured.
  • NVFP4 FP8-sweep kernel compile time: the kernel was autotuned over three tile configs, so first use compiled all three (~70 s per dtype on the pytorch:26.09 runner), pushing the MSE-sweep export and calibration tests past the 120 s timeout. It now uses the config that was fastest on B300 (BLOCKS_PER_PROGRAM=64, 8 warps; on Ada all three were within ~8% from 256² to 16384x8192). Cold first call drops from 13.6 s to 5.4 s locally, which also helps users' first MSE calibration in each process.
  • Stale xfails: removed the five xfail(raises=RuntimeError) markers in test_megatron_preprocess_data.py; they xpassed in all 151 PR runs and every nightly that reached them over the last week (one of them never touches HF).
  • Dedupe: removed tests/gpu_megatron/_extensions/test_torch_extensions.py, an exact copy of tests/gpu/_extensions/ (the megatron conftest already prebuilds the extensions).
  • Puzzletron e2e trimmed: Puzzletron is not actively maintained, so CI runs only Nemotron-3-Nano (hybrid MoE), gpt-oss and Qwen3-VL; the other six models are manual (~40 s each, ~4 min of the gpu job). Run them with --run-manual.
  • test_mamba_search_space timeout 120 -> 150 s: across ~135 PR gpu_megatron jobs on Oct 16-17 it usually took 64-76 s, ~15% took 85-103 s on slow runners, and it reached 112, 119 and 123 s (twice; one timed out here). Nightlies were ~72 s.

Testing

  • Pool: the new test (rank 0 fails, rank 1 hangs, teardown barrier) fails without the fix (queue.Empty after the 30 s job timeout) and passes with it; CPU unit tests that use the shared pool pass.
  • Cache helper: stub-session check, cold cache runs pip wheel per missing package, warm cache runs none and installs the cached wheels.
  • Dispatched on this branch: Regression tests on pytorch:26.09 passed; Unit tests passed. PR CI on 9d9ab38: everything green except gpu (2 FP8-sweep timeouts, fixed above); gpu_megatron passed in 32.5 min (60 min limit); the torch-extension cache was restored.
  • FP8 sweep: tests/gpu/torch/quantization/test_nvfp4_fp8_sweep_kernel.py and the accelerate MSE tests pass locally (Ada, torch 2.14); the hf_ptq.py export case was not run locally.
  • PR CI on f222d46: all checks green except gpu_vllm (all 190 tests passed, then hit the old 15 min limit) and gpu_megatron (test_mamba_search_space at 123 s, timeout raised above). The gpu job restored the wheelhouse and installed all three wheels without compiling.
  • FSDP: tests/unit/torch/quantization/test_utils.py passes on CPU; GPU FSDP2/layerwise/export tests pass in nemo:26.08 (56 passed, 1 skipped).
  • Puzzletron/mamba: in nemo:26.08 the six puzzletron models skip as manual and test_mamba_search_space passes with the new marker.
  • Not yet confirmed: that the per-job extension keys fill on the first run and are reused on the next.
  • Eagle mode: torch._inductor.list_mode_options confirms max-autotune sets triton.cudagraphs and the new mode does not; tests/unit/torch/speculative (339) and tests/gpu/torch/speculative (14) pass.

Looked at but left alone: the 11 unified-export cases (each hits a different fuse-flag branch). Shared worker pools across megatron modules and a preloaded forkserver (importing modelopt initializes CUDA) did not work out.

  • Is this change backward compatible?: ✅
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: ✅
  • Did you update Changelog?: N/A
  • Did you get Claude approval on this PR?: ❌

🤖 Generated with Claude Code

kevalmorabia97 and others added 3 commits October 16, 2026 04:07
…r test to unit

A failing rank blocked in megatron_worker_teardown's barrier never reported, so the 30 s failure grace
period could not start and the job waited for the pytest timeout instead. Also doc/comment fixes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
- gpu job: pin and cache mamba_ssm / causal-conv1d / fast-hadamard-transform wheels per container image
  (~10 min of compilation per run); noxfile builds a wheel only if the wheelhouse lacks it
- Bump pytorch 26.05/26.07 -> 26.09, trtllm 1.3.0rc20 -> rc29, vllm v0.30.0 -> v0.31.0 (legacy v0.20.0 kept);
  nemo 26.08 is already the newest public. The tensorrt:26.05 images used by the ONNX/diffusers examples stay
  (torch-tensorrt needs libnvinfer.so.10)
- benchmark.py: tolerate TensorRT builds without NetworkDefinitionCreationFlag.EXPLICIT_BATCH (removed in 11)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
tests/gpu_megatron/_extensions/test_torch_extensions.py was identical to tests/gpu/_extensions/; the megatron conftest already prebuilds the extensions.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97
kevalmorabia97 requested review from a team as code owners October 16, 2026 11:08
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/Model-Optimizer/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 003096da-2c11-4858-88da-ee5e5dca6398
📥 Commits

Reviewing files that changed from the base of the PR and between e0dafde and 27235c7.

📒 Files selected for processing: 6
  • .github/workflows/gpu_tests.yml
  • modelopt/onnx/quantization/autotune/benchmark.py
  • noxfile.py
  • tests/_test_utils/torch/distributed/utils.py
  • tests/gpu/torch/utils/test_distributed_worker_pool.py
  • tests/unit/torch/utils/test_get_live_pool.py
🚧 Files skipped from review as they are similar to previous changes: 1
  • tests/unit/torch/utils/test_get_live_pool.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The pull request updates test container images and cache workflows, adds local NVFP4 scale conversion, changes TensorRT flag handling and Eagle’s compile mode, and reports distributed-worker errors before teardown. Related tests and fixture documentation are updated.

Changes: CI images and dependency caches

Layer / File(s) Summary
Test container versions
.github/workflows/example_tests.yml, .github/workflows/gpu_tests.yml, .github/workflows/regression_tests.yml
Updates PyTorch and TensorRT-LLM container versions used in test jobs.
GPU dependency wheel builds and cache
noxfile.py, .github/workflows/gpu_tests.yml
Builds or reuses pinned GPU dependency wheels and installs them in the GPU session. Adds wheelhouse cache restore and save steps.
Extension cache restore and save
.github/actions/cache-extensions/action.yml, .github/actions/save-extensions/action.yml, .github/workflows/_example_tests_runner.yml, .github/workflows/gpu_tests.yml
Exposes extension cache hit and key outputs, adds a cache save action, and wires cache saves into example and GPU workflows.

NVFP4 scale conversion

Layer / File(s) Summary
Local NVFP4 scale conversion
modelopt/torch/quantization/qtensor/nvfp4_tensor.py, modelopt/torch/export/unified_export_hf.py, tests/gpu_trtllm/torch/quantization/backends/test_nvfp4_gemm.py, tests/unit/torch/quantization/test_nvfp4_tensor.py
Adds a local CUTLASS scale-layout converter and uses it in export and dequantization. Adds conversion tests and imports the helper from ModelOpt in the GPU test.

Distributed worker pool

Layer / File(s) Summary
Report worker errors before teardown
tests/_test_utils/torch/distributed/utils.py
Sends job errors before teardown and avoids replacing or duplicating the reported job error with a teardown error.
Worker failure and pool tests
tests/gpu/torch/utils/test_distributed_worker_pool.py, tests/unit/torch/utils/test_get_live_pool.py, tests/gpu_megatron/conftest.py
Tests failure reporting while teardown blocks and tests live-pool reuse and broken-pool replacement. Updates fixture-scope documentation.

Runtime compatibility and compile mode

Layer / File(s) Summary
Conditional TensorRT network flags
modelopt/onnx/quantization/autotune/benchmark.py
Adds EXPLICIT_BATCH only when TensorRT exposes the flag. The strongly typed flag remains enabled.
Eagle forward compile mode
modelopt/torch/speculative/plugins/hf_eagle.py
Changes _eagle_forward to use max-autotune-no-cudagraphs. Other compile targets are unchanged.

GPU test references

Layer / File(s) Summary
GPU test comments and references
tests/gpu/torch/speculative/plugins/test_hf_dflash.py, tests/gpu_megatron/_extensions/test_torch_extensions.py, tests/gpu_megatron/conftest.py
Revises the DFlash layer-placement comment, removes an extension-test forwarding file, and updates the documented extension-test location.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Other

Sequence Diagram(s)

sequenceDiagram
  participant GPUWorkflow
  participant WheelhouseCache
  participant NoxGPU
  GPUWorkflow->>WheelhouseCache: restore wheelhouse using image and noxfile key
  GPUWorkflow->>NoxGPU: run GPU session with MODELOPT_WHEELHOUSE
  NoxGPU->>WheelhouseCache: reuse or build and install selected wheels
  GPUWorkflow->>WheelhouseCache: save wheelhouse after cache miss and completion
Loading
sequenceDiagram
  participant TestWorkflow
  participant CacheExtensions
  participant SaveExtensions
  TestWorkflow->>CacheExtensions: restore extensions
  CacheExtensions-->>TestWorkflow: return cache hit and key
  TestWorkflow->>SaveExtensions: save extensions after cache miss
Loading

Suggested reviewers: chenhanyu, shengliangxu

Merge Risk: ⚪ Minimal · up to 27235

This change updates CI images, caches, and compatibility handling. No actionable merge-blocking risk remains after review.

Pre-merge checks: Passed 5 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 46.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 12 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 5 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns Passed No listed security anti-pattern was introduced. The PR changes four Python files under modelopt and no Python files under examples. Their added diff lines contain no `torch.load(..., weights_only=…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the main CI changes: caching source-built wheels and extensions, updating CI containers, and reducing GPU test overhead. It is specific and related to the changeset.
Full details: Security Anti-Patterns

Explanation

No listed security anti-pattern was introduced. The PR changes four Python files under modelopt and no Python files under examples. Their added diff lines contain no torch.load(..., weights_only=False), numpy.load(..., allow_pickle=True), hardcoded trust_remote_code=True, eval()/exec(), or # nosec. No pyproject.toml or requirements.txt file changed.

Full details: Docstring Coverage

Explanation

Docstring coverage is 46.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 12 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97

Copy link
Copy Markdown
Collaborator Author

/claude review

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://NVIDIA.github.io/Model-Optimizer/pr-preview/pr-2836/

Built to branch gh-pages at 2026-10-17 10:57 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.74%. Comparing base (0df6ab3) to head (f1311ea).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
- Coverage   76.89%   75.74%   -1.16%     
==========================================
  Files         658      658              
  Lines       73649    75110    +1461     
==========================================
+ Hits        56636    56891     +255     
- Misses      17013    18219    +1206     
Flag Coverage Δ
unit 60.44% <84.61%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The nightly gpu job never found its extension cache in a month of runs: actions/cache saves only when the job succeeds, and the nightly was failing or timing out every night, so the main-branch cache was never populated.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Comment thread noxfile.py Outdated
Comment thread .github/workflows/gpu_tests.yml
Comment thread noxfile.py Outdated
Comment thread tests/_test_utils/torch/distributed/utils.py
Comment thread .github/workflows/gpu_tests.yml
Comment thread .github/workflows/example_tests.yml Outdated
Comment thread tests/gpu/torch/utils/test_distributed_worker_pool.py
Comment thread tests/unit/torch/utils/test_get_live_pool.py
Comment thread modelopt/onnx/quantization/autotune/benchmark.py Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review — CI follow-up PR

Scope: full review (the trigger comment was a bare /claude review). 11 changed files, all reviewed: modelopt/onnx/quantization/autotune/benchmark.py, noxfile.py, the three workflow files, and the five test files. Note that a two-dot git diff origin/main HEAD in the shallow CI checkout surfaces unrelated main-only changes (the external-draft removal, find_final_norm_path); I scoped the review to the 11 files GitHub reports as this PR's, and ignored that noise.

Findings — CRITICAL: 1, IMPORTANT: 4, SUGGESTION: 3

Most impactful

1. The wheel cache probably doesn't work at all (CRITICAL, noxfile.py:131). The workflow caches /root/.cache/wheelhouse and never sets MODELOPT_WHEELHOUSE, so the noxfile falls back to ~/.cache/wheelhouse. In a GitHub Actions container job HOME is remapped to /github/home, not the image's /root — so the restore fills a directory nothing reads, the glob always misses, and all three packages recompile on every run. That is precisely the ~10 min this PR exists to remove. The sibling .github/actions/cache-extensions avoids this by writing the literal path into $GITHUB_ENV rather than relying on ~; doing the same here is a one-line fix. A single echo "$HOME" in the gpu job confirms or refutes it before merge.

2. A partial wheelhouse can be frozen into the cache key permanently (IMPORTANT, gpu_tests.yml:104). always() is justified for a failing test, but a timeout can also land between wheel builds. Saving a one-of-two wheelhouse, then skipping the save on every subsequent run because cache-hit == 'true', leaves the missing wheel rebuilt forever — and the key only changes when noxfile.py does, so it cannot self-heal. Suggested either a completeness marker or run_id-suffixed keys with restore-keys.

3. The container bump collides with the repo's own cppimport note (IMPORTANT, gpu_tests.yml:47). noxfile.py still says tests/gpu/_extensions/test_onnx_extensions.py "fails for newer containers until cppimport#98", and that test is an unconditional assert with no skip marker. 26.05 → 26.09 is squarely "newer". Either the note is stale and should be deleted in this PR, or the test needs a guard. The PR body says the workflows have not yet been run on the new images, so this is unverified either way.

4. Teardown failures are now silently swallowed on the failure path (IMPORTANT, distributed/utils.py:200-210). The early put is the right fix for the reported hang, but the finally block still assembles a [teardown] … traceback that the if not reported guard then discards. That diagnostic is most valuable exactly in the gpu_megatron scenario this PR targets, where the failing rank's own teardown barrier is the thing that blocks.

5. The vLLM pin the noxfile says must stay in sync is now out of sync (IMPORTANT, example_tests.yml:151). Workflows go to v0.31.0; examples/vllm_serve/Dockerfile and its README stay at 0.30.0, so CI validates a version users building the shipped Dockerfile don't get.

Also raised three non-blocking SUGGESTIONs: glob prefix-collision and a bare IndexError in _install_built_from_source, a thin 5 s timing margin in the new pool test, and a 2024 copyright year on a file added in 2026.

Assessment

The pool fix itself is correct and well-targeted — reporting before teardown is the right resolution for a teardown barrier that blocks against a still-running peer, and the new test genuinely fails without it and terminates fast rather than hanging CI. The EXPLICIT_BATCH guard is right and consistent with the other create_network call site. The dedupe of the symlinked megatron extension test is sound given the conftest already prebuilds the extensions.

Risk: medium, and concentrated in the CI plumbing rather than the library. Nothing here can affect user-facing behavior — benchmark.py is the only modelopt/ change and it only widens TensorRT version tolerance. But the headline benefit of the PR (the wheel cache) looks likely to be a no-op as written, and two of the container bumps are unverified against the images. Running the gpu job on this branch would resolve findings 1 and 3 together and is worth doing before merge.

mode="max-autotune" enables inductor CUDA graphs, whose recording intermittently fails in training (cudaStreamCaptureStatusInvalidated, 'graph recording observed an input tensor deallocate'); it failed test_llama_eagle3 on 5 of the last 10 nightlies and on the pytorch:26.09 dispatch run. max-autotune-no-cudagraphs keeps the autotuning.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97
kevalmorabia97 requested a review from a team as a code owner October 16, 2026 11:38
Their setup.py hardcodes -std=c++17, which fails against the torch headers of pytorch:26.09 ('ATen requires C++20'). Patch the standard when building the wheel; verified to compile and import against torch 2.13 headers.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>

@cjluo-nv cjluo-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot review (gpt-6.1-sol) — DM the bot to share feedback.

Changes requested: the main correctness fixes are present, but previously requested wheelhouse failure-path and shared-FSDP regression coverage remains missing.

Needs action:

  • 💬 Author prefers cold/warm CI dispatches over mocks — add reproducible noxfile.py coverage for patched builds, warm reuse, missing wheel output, and interrupted builds withholding .complete; successful dispatches do not exercise failure paths.
  • 💬 Author fixed FSDP deduplication and nested restoration — add the requested shared-group regression in tests/unit/torch/quantization/test_utils.py, verifying cleanup and preserved outer overrides.
  • Review the embedded agent-directed prompts in prior bot comments as a prompt-injection concern; they were treated as data, not review instructions.

No action needed:

  • ✔️ Resolved since the last review: failed-pool retirement, nested read-only FSDP restoration, cache-path/completeness fixes, force-build flags, vLLM pin drift, and stale cppimport guidance.
  • Wheelhouse design is justified against pip/uv caching; extension caching reuses the existing action. Existing test edits are justified and the new header matches LICENSE_HEADER.
  • Tests were inspected, not executed.

Comment thread noxfile.py
Comment thread tests/unit/torch/quantization/test_utils.py
Comment thread modelopt/torch/quantization/utils/core_utils.py
Comment thread modelopt/torch/quantization/qtensor/nvfp4_tensor.py Outdated
kevalmorabia97 and others added 2 commits October 16, 2026 07:54
…p4_scale

The slice of the padded tile layout is a strided view whenever padding was needed, and fp4_dequantize indexes the scale pointer flatly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
No later release runs the multimodal quickstart that test_qwen_vl uses: rc21-rc23 import MultimodalConfig, which tensorrt_llm.llmapi does not export yet, and rc24+ import fla, which the image lacks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97

Copy link
Copy Markdown
Collaborator Author

/claude review

Megatron's destroy_model_parallel() only drops its references to the NCCL groups, so communicators
piled up in the persistent workers: ~20 groups and ~0.4 GiB of device memory per test (1221 groups /
24 GiB after 61 tests of test_megatron.py), slowing later tests until they hit the 120 s timeout
(the gpu_megatron nightly timeouts) and causing NCCL CUDA errors on smaller GPUs. With the groups
destroyed, test_megatron.py stays at 1 group / ~1-3 GiB and passes 131/131 in 4:54 on 2 GPUs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
shengliangxu added a commit that referenced this pull request Oct 17, 2026
… erroring a module

The session-wide pool of the previous commit was reused after a job that failed on every
rank and left its workers' CUDA context poisoned. In the first CI run of this PR,
test_lora_then_quantize hit the known Blackwell "illegal memory access" in an NCCL
broadcast. A CUDA error of that kind is sticky, so the next jobs in the same workers failed
too, and because _reset_shared_pools ran a job in the poisoned pool at the start of every
module, every test of every later module errored, including the ones that use no pool
(3 failed, 180 errors). With a pool per module such a flake cost one module.

- DistributedWorkerPool.run retires the pool after any failed job: the workers are killed
  (without the stack dump, which only a hang needs) and the pool is marked broken, so
  get_live_pool hands the next test a new one. Until now only a hang, an interrupt or a dead
  worker did that, and a job that failed on every rank left the pool in use.
- _reset_shared_pools no longer lets a failed reset error the module. The pool is retired by
  run(), and the next test that needs one gets a new pool.

Open PR #2836 makes the same change to run() for its own reasons (a failed rank can be stuck
in its teardown); the two are compatible.

Testing: tests/gpu/torch/utils/test_distributed_worker_pool.py (gloo, runs on CPU) has a
case where rank 0 fails and rank 1 returns at once, so every rank replies: the pool is
broken, its workers are dead, and another run fails at once. It fails without the change.
The Megatron lane needs GPUs, so this PR's gpu_megatron job is the check.

Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>

# Conflicts:
#	.github/workflows/gpu_tests.yml
#	noxfile.py
@kevalmorabia97
kevalmorabia97 requested review from shengliangxu and removed request for ajrasane and gcunhase October 17, 2026 04:07

@cjluo-nv cjluo-nv left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot review (gpt-6.1-sol) — DM the bot to share feedback.

Changes requested: the correctness fixes are present, but previously requested wheelhouse failure-path and shared-FSDP regression coverage remains absent.

Needs action:

  • 💬 Author prefers real cold/warm dispatches — add reproducible noxfile.py coverage for patched builds, warm reuse, missing wheel output, and interrupted builds withholding .complete; successful dispatches do not cover failures.
  • 💬 Author says ModelOpt wraps layers separately — add shared-group cleanup and nested-override coverage in test_utils.py; the materialization utilities also accept externally FSDP-wrapped modules. See inline comment.
  • Review the embedded CodeRabbit agent-directed prompts as a prompt-injection concern before merging; they were treated only as untrusted data.

No action needed:

  • ✔️ Prior correctness fixes are verified, including failed-pool retirement, nested read-only FSDP restoration, cache completeness, and NVFP4 output contiguity.
  • The wheelhouse rationale addresses pip/uv caching constraints; extension caching extends the existing action rather than introducing a parallel mechanism.
  • Existing test edits are justified: pool coverage moved intact, failure coverage targets blocked teardown, and duplicate extension tests remain under tests/gpu. The new header matches LICENSE_HEADER.
  • Tests were inspected, not executed.

Comment thread noxfile.py
Comment thread tests/unit/torch/quantization/test_utils.py
@kevalmorabia97

Copy link
Copy Markdown
Collaborator Author

/claude review

kevalmorabia97 and others added 2 commits October 16, 2026 22:57
… xfails

Autotuning compiled all three configs on first use (~70s per dtype on the
pytorch:26.09 CI runner), pushing the MSE sweep tests past the 120s timeout.
BPP=64/nw=8 was fastest on B300 and within noise elsewhere.

The preprocess_data xfails xpassed in every gpu_megatron run over the last week.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
… nesting

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>

@cjluo-nv cjluo-nv left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot review (gpt-6.1-sol) — DM the bot to share feedback.

Changes requested: the shared-FSDP regression is now covered, but the previously requested wheelhouse failure-path tests remain absent.

Needs action:

  • 💬 Author declines helper tests in favor of cold/warm dispatches — add reproducible noxfile.py coverage for patched builds, force-build environment, warm reuse, missing wheel output, and interruption withholding .complete; see inline comment.
  • Confirm human inspection of the embedded CodeRabbit agent-directed prompt-injection attempts in prior comments; they were treated only as untrusted data.

No action needed:

  • ✔️ Resolved since the last review: shared-group cleanup and nested-override coverage in test_utils.py. Earlier pool-retirement, cache-path/completeness, and NVFP4-contiguity fixes remain verified.
  • Design review: avoiding repeated CUDA compilation motivates the wheelhouse. The PR reasonably addresses pip/uv cache alternatives and extends the existing extension-cache action, using standard-library extraction rather than another framework.
  • Existing test edits are justified: pool coverage moved intact, duplicate extension coverage remains under tests/gpu, and stale xfail removal strengthens coverage. The new header matches LICENSE_HEADER.
  • Tests were inspected, not executed.

Comment thread noxfile.py

CI caches the wheelhouse (``MODELOPT_WHEELHOUSE``) per container image, so only the first run on
an image builds.
"""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot comment.

The cold/warm dispatch evidence establishes real builds and reuse, but does not resolve the previously requested failure-path coverage. Please add a compact test with a temporary wheelhouse, tiny local sdist, and recording/failing session boundary. Run the actual extraction/patching logic and verify C++20 source plus package-specific force-build environment reach pip wheel, warm reuse skips builds, missing wheel output raises the named RuntimeError, and a build/install failure before completion leaves .complete absent. This need not compile CUDA or mock tarfile internals; it tests observable cache-publication contracts rather than restating implementation lines.

shengliangxu added a commit that referenced this pull request Oct 17, 2026
Each of the 15 gpu_megatron modules that use `dist_workers` started its own
worker pool, about 19 of the lane's 36 minutes. Fourteen logged module starts
took 75-77 s each (the GDN module 179 s: 76 s pool plus 103 s Triton JIT), plus
3.6-10 s of shutdown, and the start has a p90 of 94-98 s.

The 76 s is not distributed init (an in-process init takes 0.1 s): the spawned
child unpickles megatron_worker_teardown by reference, which imports the heavy
tests/gpu_megatron/conftest.py (transformers, megatron.core, all of modelopt and
its plugins, TE) before it reaches init_process_group. The same import chain
takes pytest itself 89 s to start here against 17 s in the gpu lane, whose
session-scoped pool, with a light teardown function, starts in 1.7 s. Module scope
was chosen when a spawn cost about 10 s ("avoids cross-test state contamination");
the git history shows no contamination incident, and the cost has grown 7.6x.

_pool_cache is now session-scoped, so the pools are keyed by world size and
shared by every module. The dist_workers fixtures stay function-scoped and get
their pool from get_live_pool, which replaces a pool that a hang killed. What
isolation the module scope gave is kept explicitly:
- megatron_worker_teardown, after every job: synchronizes CUDA first (an
  asynchronous CUDA error then surfaces in the test that caused it), destroys
  Megatron's model-parallel state as before, and now also the process groups it
  leaves behind (about 20 NCCL groups per dispatch, each with a communicator and a
  watchdog thread; test_megatron.py alone dispatches about 110 times). The loop is
  the one open PR #2836 adds for the same leak, comment included: whichever PR
  lands second sees an identical hunk. A pool that lives through the whole session
  needs it more than a per-module pool did;
- _reset_shared_pools, an autouse module-scoped fixture: when a module starts, it
  resets the process-global torch settings that helpers like set_seed change
  (cudnn and TF32 flags, deterministic algorithms, RNG) to what a fresh worker
  starts with (reset_worker_state, which the workers prepare by recording those
  settings right after they start), in the pools that already exist. It is not part
  of _get_pool, which runs once per test and would add a job to each;
- get_live_pool and the pool's abort handling: a job that times out, is interrupted
  or loses a worker kills the pool and the next test gets a fresh one, for the cost
  of one pool start. That also covers CUDA poisoning after the known Blackwell
  illegal-memory-access flake.

Expected saving: about 17 min of the gpu_megatron job (36 min of tests to about
19), and one pool start instead of 15 means fewer chances to hit the ready
timeout.

Testing: reset_worker_state is covered by a gloo test in
tests/gpu/torch/utils/test_distributed_worker_pool.py (it runs on CPU): flags and
seed changed in a job are back at their spawn-time values afterwards. The Megatron
lane needs GPUs, so this PR's gpu_megatron job is the check. In an earlier CI run of
the whole series (with the Triton cache of another change), the lane's pytest step
took 862 s on cold caches and 336 s on warm ones, against 2,022 s for a baseline
run.

Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
shengliangxu added a commit that referenced this pull request Oct 17, 2026
… erroring a module

The session-wide pool of the previous commit was reused after a job that failed on every
rank and left its workers' CUDA context poisoned. In the first CI run of this PR,
test_lora_then_quantize hit the known Blackwell "illegal memory access" in an NCCL
broadcast. A CUDA error of that kind is sticky, so the next jobs in the same workers failed
too, and because _reset_shared_pools ran a job in the poisoned pool at the start of every
module, every test of every later module errored, including the ones that use no pool
(3 failed, 180 errors). With a pool per module such a flake cost one module.

- DistributedWorkerPool.run retires the pool after any failed job: the workers are killed
  (without the stack dump, which only a hang needs) and the pool is marked broken, so
  get_live_pool hands the next test a new one. Until now only a hang, an interrupt or a dead
  worker did that, and a job that failed on every rank left the pool in use.
- _reset_shared_pools no longer lets a failed reset error the module. The pool is retired by
  run(), and the next test that needs one gets a new pool.

Open PR #2836 makes the same change to run() for its own reasons (a failed rank can be stuck
in its teardown); the two are compatible.

Testing: tests/gpu/torch/utils/test_distributed_worker_pool.py (gloo, runs on CPU) has a
case where rank 0 fails and rank 1 returns at once, so every rank replies: the pool is
broken, its workers are dead, and another run fails at once. It fails without the change.
The Megatron lane needs GPUs, so this PR's gpu_megatron job is the check.

Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
kevalmorabia97 and others added 6 commits October 17, 2026 01:15
Editing a Triton kernel invalidated the cache, so the gpu_vllm job rebuilt the
extensions and hit its 15-min timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
…apped

persistent_materialization gathers nothing for a layer that is not itself an
FSDPModule, so its FSDP submodules must keep unsharding (the previous behavior).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
The v0.30.0 job passed all tests with a warm extension cache but hit the
15 min limit: the image pull alone takes ~4.5 min.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
…timeout

Puzzletron is not actively maintained, so CI keeps Nemotron-3-Nano, gpt-oss
and Qwen3-VL and marks the other six models manual (~4 min of the gpu job).
test_mamba_search_space usually takes ~70s but hit 123s on slow runners.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Example and GPU jobs on the same image shared one key. An example job that
builds no extensions finished first and saved an empty cache, so the gpu and
gpu_megatron jobs restored it as an exact hit, recompiled every run, and
could never save their own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97 kevalmorabia97 changed the title CI follow-up: cache source-built wheels, bump CI containers, pool fix, drop duplicate megatron extension tests CI follow-up: cache source-built wheels and extensions, bump CI containers, trim slow GPU tests Oct 17, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants