ci(cuda): overlap AOTI .pt2 compiles with GPU unit tests#5882
ci(cuda): overlap AOTI .pt2 compiles with GPU unit tests#5882wanghan-iapcm wants to merge 4 commits into
Conversation
The CUDA job ran source/tests as one serial process, so the .pt2-freezing tests (torch.export + AOTInductor) held the step for minutes while inductor/g++/ptxas generated code on the CPU -- the GPU measured ~98% idle (<250 MiB) during those compiles. Split the pytest step into two groups that run concurrently on the one GPU: a CPU-bound compile group (-m aoti_compile, niced so the GPU group keeps CPU priority) and the GPU-bound remainder (-m "not aoti_compile"), so the compile CPU-time overlaps the GPU unit tests. The compile group's GPU footprint is negligible, so the two coexist on one device without contention; wall-clock becomes max(compile, gpu) rather than their sum. The aoti_compile marker is auto-applied in source/tests/conftest.py to any test whose module references a .pt2 freeze entry point, so the partition stays correct as tests are added without a hand-maintained file list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCUDA pytest collection now assigns an explicit ChangesAOTI CUDA test partitioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@source/tests/conftest.py`:
- Around line 36-39: Update the file-reading exception handler in the conftest
collection logic to catch UnicodeDecodeError alongside OSError, preserving the
existing fallback of assigning an empty string to src for unreadable or
non-UTF-8 files.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abaf51b2-a9cb-488f-879f-939eaa65a1a4
📒 Files selected for processing (3)
.github/workflows/test_cuda.ymlpyproject.tomlsource/tests/conftest.py
| try: | ||
| src = Path(path).read_text(encoding="utf-8") | ||
| except OSError: | ||
| src = "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Catch UnicodeDecodeError when reading file contents.
If pytest collects a non-text file or a text file with a different encoding (e.g., a compiled library or binary data file), Path.read_text(encoding="utf-8") will raise a UnicodeDecodeError. Since UnicodeDecodeError inherits from ValueError and not OSError, it will bypass the current except block and crash the test collection process.
Catch UnicodeDecodeError alongside OSError to safely fall back to an empty string for non-UTF-8 files.
🛠️ Proposed fix
- try:
- src = Path(path).read_text(encoding="utf-8")
- except OSError:
+ try:
+ src = Path(path).read_text(encoding="utf-8")
+ except (OSError, UnicodeDecodeError):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| src = Path(path).read_text(encoding="utf-8") | |
| except OSError: | |
| src = "" | |
| try: | |
| src = Path(path).read_text(encoding="utf-8") | |
| except (OSError, UnicodeDecodeError): | |
| src = "" |
🤖 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 `@source/tests/conftest.py` around lines 36 - 39, Update the file-reading
exception handler in the conftest collection logic to catch UnicodeDecodeError
alongside OSError, preserving the existing fallback of assigning an empty string
to src for unreadable or non-UTF-8 files.
|
The overlap direction is worth exploring, but I don't think the current automatic The marker is inferred by grepping source text, which is not equivalent to “this test actually triggers a
Could we instead mark the concrete Keeping separate JUnit reports is a good part of the implementation. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.6-terra) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5882 +/- ##
==========================================
- Coverage 78.83% 78.56% -0.27%
==========================================
Files 1054 1054
Lines 121758 121758
Branches 4410 4411 +1
==========================================
- Hits 95983 95663 -320
- Misses 24215 24528 +313
- Partials 1560 1567 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Replace the source-grep aoti_compile auto-marker with an explicit curated list of the modules that actually freeze a .pt2, plus an always-on runtime guard. The grep both missed freeze(...)-driven compiles (test_dp_freeze, test_finetune, test_multitask, test_dp_test, test_change_bias, test_models -- none name the low-level entry points) and over-tagged files that only mock the compiler or emit .pth/.pte. The producer list was derived by running the suite under a detector that wraps the real compile entry points (aoti_compile_and_package, _trace_and_export). The guard wraps those same entry points every run and fails the session, with the offending nodeids, if any test freezes a .pt2 without the aoti_compile marker -- so the list cannot silently drift, and a compile can never escape into the GPU lane unnoticed. It runs in the CPU test_python CI too, so drift is caught cheaply rather than only in the expensive CUDA job.
- echo each lane's wall-clock ([lane aoti_compile] / [lane gpu]) so T_A and T_B (vs the ~3h46m serial baseline) appear directly in the log - cap the compile lane at TORCHINDUCTOR_COMPILE_THREADS=nproc/2 (plus nice) so the GPU lane keeps CPU for its host-side dispatch - upload junit-aoti.xml / junit-gpu.xml as artifacts for post-run inspection - share one AOTInductor compile cache across all steps via a job-level TORCHINDUCTOR_CACHE_DIR, so the C++ fixture builds (gen_*.py in test_cc_local.sh) reuse the kernels the Python lane compiled, and persist it across runs with actions/cache (10 GB repo-cache limit noted inline)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/test_cuda.yml:
- Line 36: Update the actions/checkout step in the CUDA test workflow to
explicitly disable credential persistence by setting persist-credentials to
false, while leaving the existing checkout behavior unchanged.
- Around line 97-108: Update both pytest lane subshells in the parallel test
execution block to initialize rc=0 before running pytest and append || rc=$? to
each pytest command. Preserve the existing wall-time echo and exit $rc reporting
so failures are captured without set -e bypassing the reporting.
In `@source/tests/conftest.py`:
- Around line 118-123: Update the pytest_runtest_protocol hookwrapper so the
yield executes inside a try...finally block, and move the _current_item["item"]
reset into the finally clause to guarantee cleanup when the wrapped hook raises.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d10c758-b81a-495e-9f64-75a7057c1315
📒 Files selected for processing (2)
.github/workflows/test_cuda.ymlsource/tests/conftest.py
| # options: --gpus all | ||
| if: github.repository_owner == 'deepmodeling' && (github.event_name == 'pull_request' && github.event.label && github.event.label.name == 'Test CUDA' || github.event_name == 'workflow_dispatch' || github.event_name == 'merge_group') | ||
| steps: | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Disable credential persistence to improve workflow security.
The actions/checkout action persists the GITHUB_TOKEN in the local git config by default. If untrusted code or tests are executed within this environment, the token could be exposed. It is a security best practice to explicitly set persist-credentials: false unless the workflow requires authenticating subsequent git commands (like pushing a commit).
🛡️ Proposed fix
- - uses: actions/checkout@v7
+ - uses: actions/checkout@v7
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v7 | |
| - uses: actions/checkout@v7 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 36-36: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/test_cuda.yml at line 36, Update the actions/checkout step
in the CUDA test workflow to explicitly disable credential persistence by
setting persist-credentials to false, while leaving the existing checkout
behavior unchanged.
Source: Linters/SAST tools
|
Thanks — you're right that a source grep is the wrong basis for the scheduling boundary, on both counts. Replaced it (f660993, b4a7996). The
An always-on guard wraps the same entry points every run and fails the session (with the offending nodeids) if any test freezes a Other points: compile-thread constraint added ( Also made the inductor compile cache explicit (shared |
- CUDA lanes: init rc=0 and `|| rc=$?` so the step's default `set -e` cannot skip the per-lane wall-clock echo when a lane fails (that timing is exactly what we want on a failure). - conftest guard: wrap the hookwrapper yield in try/finally so _current_item is reset even if the wrapped hook raises, avoiding misattributed compiles in teardown hooks.
wanghan-iapcm
added
Test CUDA
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Thanks for replacing the source-grep classifier with an explicit producer list and a runtime drift guard. That fixes the main issue in the first revision. I still think this needs changes before merge.
-
The scheduling boundary remains module-level, while the safety/performance premise is test-level. Every test in each
_AOTI_COMPILE_MODULESfile is sent to theaoti_compileprocess, including tests that do GPU inference or other GPU-heavy work but do not freeze a.pt2. Both pytest processes are pinned toCUDA_VISIBLE_DEVICES=0, so the implementation can still create real GPU/VRAM contention between two independent CUDA processes. The marker/guard establishes that.pt2producers do not escape the compile lane; it does not establish that the compile lane has negligible GPU use.Please either partition at concrete test/fixture/class granularity, or provide repeated CUDA-run evidence for the current explicit partition: per-lane
--durations, GPU-utilization/VRAM traces, end-to-end wall time, and a few clean repeat runs. I would not merge based only on the earlier aggregate 98%-idle observation. -
Make the persistent Inductor cache environment-safe.
restore-keys: inductor-cuda-can restore artifacts created under a different Torch/Inductor, CUDA/toolchain, or GPU architecture. Please key the cache at least on the dependency/workflow lock inputs and the relevant Torch/CUDA environment (and GPU capability if runners may vary), rather than restoring from every historical CUDA cache. A miss is fine; cache correctness and reproducibility matter more than a hit. -
Minor security hygiene: set
persist-credentials: falseonactions/checkout, since this workflow executes PR code/tests and does not need to push.
The separate JUnit reports and the per-lane exit-code/timing handling look good.
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.6-terra)
2 participants
What
The CUDA job runs
source/testsas one serial pytest process. The tests that freeze a.pt2(torch.export + AOTInductor) are CPU-bound: inductor / g++ / ptxas generate code for minutes while the GPU sits idle. On a Tesla T4 the GPU measured ~98% idle (peak <250 MiB) for the entire duration of that test group, so those minutes are pure serialization in front of the GPU-bound tests.This splits the pytest step into two groups that run concurrently on the one GPU:
-m aoti_compile— the CPU-bound.pt2-freezing tests,niced so the GPU group keeps CPU priority-m "not aoti_compile"— the GPU-bound remainderso the compile CPU-time overlaps the GPU unit tests. The compile group's GPU footprint is negligible (<250 MiB), so both groups coexist on one device without contention. Wall-clock becomes
max(compile, gpu)instead of their sum.How the partition stays correct
The
aoti_compilemarker is auto-applied insource/tests/conftest.pyto any test whose module references a.pt2freeze entry point (deserialize_to_file/_trace_and_export/aoti_compile_and_package). No hand-maintained file list — new compiling tests are partitioned automatically. The two selections are complementary and exhaustive (verified locally: A + B = total, disjoint).Notes / limitations
max(T_A, T_B)vs the sum) depends on the runner and is measurable on the CUDA CI; overlap quality scales with the runner's core count (niceis a no-op unless the CPU saturates)..pt2-freezing tests.Summary by CodeRabbit