feat: add CUDA 13 runtime and CI coverage#656
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds CUDA 13.0 (
Confidence Score: 5/5Safe to merge; all generated artifacts are consistent with the source-of-truth TOML, and the CI rollout correctly gates on the cu130 rows as non-required. The changes are mechanical manifest additions following an already-proven pattern. The nvidia_index fallback is simple and validated end-to-end by the generator's own source-index validation step. The cu130 NVIDIA library packages ship without a CUDA-version suffix (intentional, documented), and uv source routing is correct throughout. No breaking changes to cu129 or cpu extras. No files require special attention. The vllm wheel situation for cu130 is documented and was already reviewed in a prior thread. Important Files Changed
|
| dependencies = [ | ||
| "vllm==0.24.0; sys_platform == 'linux'", | ||
| ] |
There was a problem hiding this comment.
vllm for cu130 has no dedicated wheel source
cu129 routes vllm through an explicit wheel server entry ([sources.cu129] → vllm-v0-24-0-cu129). For cu130, vllm==0.24.0 has no static source override, so uv will resolve it from the default PyPI index. Standard PyPI vllm wheels are typically CPU-only wrappers; if the PyPI wheel doesn't carry a CUDA 13 runtime, the package will install but any GPU operation will fail (or the wheel may not exist at all for 0.24.0). This is mitigated by required: false in CI, but it may cause confusing failures if someone tries mise run bootstrap-nss cu130 manually.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }} | ||
| timeout-minutes: 210 | ||
| runs-on: linux-amd64-gpu-a100-latest-1 | ||
| continue-on-error: ${{ !matrix.required }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: |
There was a problem hiding this comment.
E2E matrix missing
python-version
The gpu-smoke-test matrix explicitly sets python-version: "3.13" on each entry, but the gpu-e2e-test matrix entries contain only cuda-extra and required. If the setup-gpu-test-env action has a required python-version input, the e2e job will silently receive an empty string. The pre-PR e2e job also didn't pass python-version, so the action likely has a working default — but it may be worth aligning the two jobs to make the Python version explicit and avoid future drift.
4127e30 to
7b0ff2e
Compare
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
A council review found docs and CI comments in this PR claiming cu130 container images and full CI coverage were "enabled," while container-build.yml (which builds and publishes release images) was never touched, and the cu130 GPU e2e matrix leg silently re-bootstraps cu129 via .mise/tasks/tests.toml before running -- so it exercises cu129 twice, not cu130 once. Walk those claims back to what this PR actually delivers: manifest support plus GPU smoke-test coverage. Container build wiring and a real cu130 e2e path move to the next PR in the stack. Also documents the nvidia_index field's precedence against its sibling pytorch_index/flashinfer_index override fields, notes why cu130 has no dedicated vllm source route yet, and adds a tracking comment to the required: false matrix entries so the temporary CI exception has a stated promotion trigger. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
7b0ff2e to
8629058
Compare
Summary
Validation
Second PR in the CUDA 13 stack; depends on #655.