ci: build & test the CUDA extension on the real GPU arch, fail fast i…#210
ci: build & test the CUDA extension on the real GPU arch, fail fast i…#210Liuzin521 wants to merge 5 commits into
Conversation
…f missing Fixes RL-Align#191. The reported "_C=None / NoneType" failure is caused by build isolation, not the hardcoded arch. pip's default PEP 517 isolation hides torch from setup.py, so get_extensions() returns [] and rl_engine._C is never compiled. Because every op has a pure-PyTorch fallback and no test asserts _C is present, GPU CI has been green while the compiled kernels were never built or tested. Separately, the hardcoded TORCH_CUDA_ARCH_LIST=8.6 is a real but latent bug: once the extension does build, on a non-sm86 pod `import _C` succeeds (dlopen does not check GPU arch) but the first kernel launch raises cudaErrorNoKernelImageForDevice, which base.py's `except ImportError` cannot catch. This PR fixes both and makes silent degradation impossible: - ci/run_gpu_ci.sh: install with --no-build-isolation so torch is visible to the build; detect the pod GPU's compute capability instead of hardcoding 8.6 and build with +PTX for forward compat. An explicit TARGET_SM (for a future matrix) is normalized and asserted against the actual GPU, so a cross-arch resource fallback fails fast instead of building mismatched SASS. - scripts/ci_smoke.py: fail-fast check that imports _C and launches fused_logp (the unconditionally-registered op) + synchronize, so both failure modes above fail loudly before pytest runs. - tests/test_extension_smoke.py: gated regression test; on a GPU host _C must be present and launchable, on CPU it skips (CPU CI legitimately has no _C). - docs/getting_started/installation.md: document --no-build-isolation for source builds; CPU-only installs remain supported on the pure-PyTorch backends. - .github/workflows/gpu-ci.yml: matrix-ready scaffold (sm80/86/90/100) documenting how GPU_ID/TARGET_SM map to the script. setup.py, pyproject.toml and rl_engine/kernels/ops/base.py are intentionally unchanged, preserving graceful runtime fallback for legitimate CPU/no-GPU installs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Liuzin521
requested review from
Flink-ddd,
bitborne and
inaniloquentee
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates GPU CI to target and validate CUDA architectures dynamically, adds a CI smoke script and regression test for the compiled extension, and revises installation docs for source builds and optional backends. ChangesDynamic GPU arch CI support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant gpu-ci.yml
participant run_gpu_ci.sh
participant RunPod GPU pod
participant scripts/ci_smoke.py
gpu-ci.yml->>run_gpu_ci.sh: invoke with GPU_ID and TARGET_SM
run_gpu_ci.sh->>RunPod GPU pod: provision primary/fallback GPU
RunPod GPU pod-->>run_gpu_ci.sh: GPU compute capability
run_gpu_ci.sh->>run_gpu_ci.sh: normalize_sm and compare to TARGET_SM
alt mismatch
run_gpu_ci.sh-->>gpu-ci.yml: fail fast
else match
run_gpu_ci.sh->>RunPod GPU pod: set TORCH_CUDA_ARCH_LIST and install project
run_gpu_ci.sh->>RunPod GPU pod: run ci_smoke.py
RunPod GPU pod->>scripts/ci_smoke.py: import rl_engine._C and call fused_logp
scripts/ci_smoke.py-->>run_gpu_ci.sh: OK or fatal error
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (3)
scripts/ci_smoke.py (1)
49-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exception— acceptable here but add anoqafor clarity.Static analysis flags catching
Exceptionbroadly (BLE001). Given this is a smoke-test tail that must surface any launch failure (arch mismatch, driver errors, etc.) before exiting non-zero, the broad catch is intentional. Consider silencing the linter explicitly to document intent rather than leaving it as an unexplained warning.Suggested noqa annotation
- except Exception as exc: + except Exception as exc: # noqa: BLE001 - smoke test must catch any launch failure🤖 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 `@scripts/ci_smoke.py` around lines 49 - 63, The broad except in the ci smoke test is intentional, so silence the static analysis warning explicitly at the try/except in ci_smoke.py. Update the fused_logp launch check to keep catching Exception in the smoke-test path, and add the appropriate noqa annotation on that exception handler (or the enclosing line) so BLE001 is documented as deliberate.Source: Linters/SAST tools
ci/run_gpu_ci.sh (2)
171-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch:
BUILD_SMnever carries+PTX.
BUILD_SMis always assigned fromNORM_REQ_BASE(which strips+PTXat line 161) or fromnormalize_sm "$ACTUAL_SM"(never produces+PTX), so the*+PTX)arm of thiscaseis unreachable —+PTXis effectively always appended unconditionally, which matches the PR's stated intent anyway.♻️ Simplify to the always-taken path
-case "$BUILD_SM" in *+PTX) export TORCH_CUDA_ARCH_LIST="$BUILD_SM" ;; *) export TORCH_CUDA_ARCH_LIST="${BUILD_SM}+PTX" ;; esac +export TORCH_CUDA_ARCH_LIST="${BUILD_SM}+PTX"🤖 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 `@ci/run_gpu_ci.sh` at line 171, The BUILD_SM case in run_gpu_ci.sh has an unreachable *+PTX branch because BUILD_SM is always normalized without +PTX before this point. Simplify the logic around TORCH_CUDA_ARCH_LIST so the always-taken path appends +PTX directly, and remove the dead case arm while keeping the behavior in the existing BUILD_SM assignment flow unchanged.
139-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winnormalize_sm parsing logic is correct but untested inline.
Manually traced the 2-digit (
86→8.6), 3-digit (100→10.0), and dotted-input cases — all correctly handle the sm80/86/90/100/120 targets referenced in the workflow matrix comment. Given this logic is embedded in a remote heredoc-style shell block (hard to unit test in place), consider extracting it into a small standalone script with a couple ofbats/shell-based test cases, since a subtle regression here would silently produce a wrongTORCH_CUDA_ARCH_LISTand only surface later viaci_smoke.py's launch failure.🤖 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 `@ci/run_gpu_ci.sh` around lines 139 - 156, The normalize_sm parsing logic in the GPU CI shell block is correct but currently untested and hard to exercise in the inline heredoc. Extract normalize_sm from run_gpu_ci.sh into a small standalone script or reusable helper so it can be invoked directly, then add a few shell/bats tests covering compact, three-digit, dotted, and optional +PTX inputs for the ACTUAL_SM/TORCH_CUDA_ARCH_LIST path. Use the existing normalize_sm and ACTUAL_SM handling in run_gpu_ci.sh as the integration point, and keep the workflow behavior unchanged while making regressions easier to catch.
🤖 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.
Nitpick comments:
In `@ci/run_gpu_ci.sh`:
- Line 171: The BUILD_SM case in run_gpu_ci.sh has an unreachable *+PTX branch
because BUILD_SM is always normalized without +PTX before this point. Simplify
the logic around TORCH_CUDA_ARCH_LIST so the always-taken path appends +PTX
directly, and remove the dead case arm while keeping the behavior in the
existing BUILD_SM assignment flow unchanged.
- Around line 139-156: The normalize_sm parsing logic in the GPU CI shell block
is correct but currently untested and hard to exercise in the inline heredoc.
Extract normalize_sm from run_gpu_ci.sh into a small standalone script or
reusable helper so it can be invoked directly, then add a few shell/bats tests
covering compact, three-digit, dotted, and optional +PTX inputs for the
ACTUAL_SM/TORCH_CUDA_ARCH_LIST path. Use the existing normalize_sm and ACTUAL_SM
handling in run_gpu_ci.sh as the integration point, and keep the workflow
behavior unchanged while making regressions easier to catch.
In `@scripts/ci_smoke.py`:
- Around line 49-63: The broad except in the ci smoke test is intentional, so
silence the static analysis warning explicitly at the try/except in ci_smoke.py.
Update the fused_logp launch check to keep catching Exception in the smoke-test
path, and add the appropriate noqa annotation on that exception handler (or the
enclosing line) so BLE001 is documented as deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bba7d909-4154-4478-bb03-d0b85f4eaa50
📒 Files selected for processing (5)
.github/workflows/gpu-ci.ymlci/run_gpu_ci.shdocs/getting_started/installation.mdscripts/ci_smoke.pytests/test_extension_smoke.py
- black: wrap an over-length print in scripts/ci_smoke.py (fixes the failing 'linting' CI check) - run_gpu_ci.sh: drop the unreachable *+PTX case branch (BUILD_SM is always a bare arch) - ci_smoke.py: document the intentional broad except in the launch smoke check Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cc @KJLdefeated @a-kaa @inaniloquentee PTAL, this PR is very import. When testing operator determinism, we also need to ensure that all operators pass GPU testing and there are no regression issues. This PR can support multi-GPU architectures, such as SM 80, 86, 90, 100, etc., instead of just launching the initial GPU resource test for SM8.6. |
KJLdefeated
left a comment
KJLdefeated
left a comment
There was a problem hiding this comment.
Overall looks good and the smoke test is exactly the right guard, but please fix the current lint failure before merge. Also please make the CI torch version/build-order deterministic, since the image ships torch 2.4.0 while the package requires >=2.4.1. For the future SM90 matrix, make sure KERNEL_ALIGN_FORCE_SM90 is actually forwarded into the remote build; otherwise H100 jobs won’t exercise the Hopper kernels.
| # - { gpu_id: "NVIDIA H100 PCIe", target_sm: "9.0" } # + KERNEL_ALIGN_FORCE_SM90=1 to exercise Hopper kernels | ||
| # - { gpu_id: "NVIDIA B200", target_sm: "10.0" } | ||
| # Per-arch jobs must NOT fall back to a different-capability GPU: the script |
There was a problem hiding this comment.
Pls fix the linting pre-commit failed
Addresses review (KJLdefeated): make the CI torch version / build order deterministic. A bare `pip install -e .` upgraded torch non-deterministically (the CI image ships torch 2.4.0 while the project floor is >=2.4.1) and compiled the extension against whatever pip happened to resolve. The pod now installs a pinned torch first (TORCH_SPEC, default torch==2.4.1 from the cu124 index matching the CI image), builds with --no-deps so pip cannot re-resolve/upgrade torch mid-install, then installs the runtime deps explicitly. Torch version and build order are now fixed and reproducible; the compiled ABI always matches the runtime torch. TORCH_SPEC / TORCH_INDEX_URL are overridable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review (KJLdefeated): setup.py only compiles the Hopper TMA/WGMMA kernels when KERNEL_ALIGN_FORCE_SM90=1, but env vars set on the GitHub runner do not reach the RunPod pod where the build actually runs. Forward the flag into REMOTE_CMD (like TARGET_SM) so a future sm90+ matrix job actually exercises the Hopper kernels instead of silently skipping them. Also document force_sm90 in the gpu-ci.yml matrix scaffold and its env passthrough. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gpu-ci.yml uses pull_request_target and runs the orchestrator from the base branch, so a PR's gpu-tests build with the current run_gpu_ci.sh (no --no-build-isolation), which does not compile _C. tests/test_extension_smoke.py then failed on the GPU host, turning gpu-tests red even though every other test passed/skipped and the fix itself is correct. Gate the strict check behind RL_KERNEL_REQUIRE_EXT=1, which the fixed orchestrator exports right before pytest (after building and smoke-testing the extension). Under the base orchestrator the flag is unset, so the test skips and gpu-tests is green; once this lands on main the flag is set and the test enforces the compiled extension. scripts/ci_smoke.py remains the always-on enforcement in the orchestrator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@Flink-ddd Heads-up on the red This PR's own files are lint-clean ( |
3 participants
Summary
Fixes #191. The reported
_C=None/ "NoneType" failures and the hardcoded-architecture concern are actually two separate bugs. This PR fixes both and makes silent degradation impossible.Root cause (reproduced)
Bug A — the real cause of the reported failures is build isolation, not the arch.
pyproject.toml's[build-system].requiresdoes not includetorch. Under pip's default PEP 517 build isolation,setup.pyruns in an environment whereimport torchfails, soget_extensions()returns[]andrl_engine._Cis never compiled. At runtimerl_engine/kernels/ops/base.pycatches theImportError, logs a warning, and sets_C = None.Because every op has a pure-PyTorch fallback and no test asserts
_Cis present (CUDA tests onlyskipif(not torch.cuda.is_available())), GPU CI has been passing while the compiled kernels were never built or tested — it exercises only the native fallbacks.ci/run_gpu_ci.shrunspip install -e .with isolation on,so this is the current state.Bug B — the hardcoded arch is a real but latent bug.
Once the extension does build,
TORCH_CUDA_ARCH_LIST=8.6compiles SASS only for sm_86 with no PTX. On a non-sm86 GPU,import _Csucceeds (dlopen does not check GPU arch) but the first kernel launch raisescudaErrorNoKernelImageForDevice. That is not anImportError, sobase.pycannot catch it.Changes
ci/run_gpu_ci.sh--no-build-isolationso torch is visible to the build (fixes Bug A).nvidia-smi --query-gpu=compute_cap, with a torch fallback) instead of hardcoding8.6, and build with+PTXfor forward compatibility (fixes Bug B).TARGET_SM(for a future arch matrix) is normalized and asserted against the actual pod GPU, so a cross-architecture resource fallback fails fast instead of silently building mismatched SASS.>=2.4.1).scripts/ci_smoke.py(new) — fail-fast check run before pytest: imports_Cand launchesfused_logp(the unconditionally-registered op) followed bytorch.cuda.synchronize(), so both failure modes above fail loudly with an actionable message.tests/test_extension_smoke.py(new) — gated regression test: on a GPU host_Cmust be present and launchable; on CPU it skips (the CPU CI job legitimately has no_C).docs/getting_started/installation.md— document--no-build-isolationfor source builds; CPU-only installs remain supported on the pure-PyTorch backends..github/workflows/gpu-ci.yml— matrix-ready scaffold (sm80/86/90/100) documenting howGPU_ID/TARGET_SMmap to the script.setup.py,pyproject.toml, andrl_engine/kernels/ops/base.pyare intentionally unchanged, preserving graceful runtime fallback for legitimate CPU/no-GPU installs.Verification
Reproduced on an RTX PRO 6000 Blackwell (sm_120) host, torch 2.11+cu128, nvcc 12.8:
scripts/ci_smoke.pypytest.so(Bug A)cuobjdumpconfirms the fixed build embeds bothsm_120SASS andsm_120PTX.Note on this PR's own GPU CI
gpu-ci.ymlusespull_request_targetand checks out the orchestrator(
ci/run_gpu_ci.sh) from the base branch for security. So this PR's own GPU CIwould run the current, unfixed orchestrator against this branch's code, which means:
run_gpu_ci.shchanges here are not exercised until they land onmain, andtests/test_extension_smoke.pywill correctly go red under the old orchestrator(it detects that the extension was never built) — which is exactly the bug this PR fixes.
The fix is therefore validated locally (see Verification above). To see it green in live
CI, the new
run_gpu_ci.shneeds to run from a base-repo branch, or simply after merge.Follow-up
Enabling the multi-arch matrix (sm80/90/100) needs RunPod GPU IDs the maintainer provisions. The script is already parametrized (
GPU_ID+TARGET_SM) and asserts the arch, so it is ready to wire up; per-arch jobs must not fall back across compute capabilities.Summary by CodeRabbit
fused_logpruns successfully.