fix(ci): make CUDA release checks hermetic - #1270
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe installer now detects CUDA from matching ChangesCUDA installer determinism
CI control-plane routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The PR makes CUDA release checks hermetic and changes workflow routing, but the current resolver can reject valid CUDA installations or select an archive without usable runtime support. Large pull requests may also be misrouted or fail because file-list truncation and pull-request read access are not handled. These concrete correctness and CI availability risks should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant pr_builds.yml
participant ControlPlanePaths
participant BootstrapOrchestrator
PullRequest->>pr_builds.yml: Provide changed pull request files
pr_builds.yml->>ControlPlanePaths: Match files against configured paths
ControlPlanePaths-->>pr_builds.yml: Return controlPlaneChanged status
pr_builds.yml->>BootstrapOrchestrator: Dispatch when controlPlaneChanged is true
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 `@install.sh`:
- Around line 359-367: Update detect_cuda_major so nvidia-smi’s CUDA Version is
treated only as a driver compatibility upper bound, not sufficient evidence to
select a CUDA archive. Require matching-major libcudart, libcublas, and
libcublasLt evidence before returning a CUDA lane, while preserving
MESH_LLM_TEST_CUDA_MAJOR fixture behavior and clamping supported majors to the
published maximum.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d72936d-5e9a-4681-a6b3-ce7e8248bf15
📒 Files selected for processing (7)
.agents/skills/manage-ci/references/current-inventory.md.github/workflows/pr_quality.ymlci/ci.mdinstall.shscripts/tests/test_install_sh.pytools/xtask/src/installer_fixtures.rstools/xtask/src/workflow_checks.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/tests/test_install_sh.py (1)
184-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit length check before
zip().
strict=Truerequires Python 3.10, but the repository does not declare that minimum and one test job invokes unpinnedpython3. Use an explicit length check to prevent silent truncation without raising the runtime requirement.🤖 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/tests/test_install_sh.py` around lines 184 - 187, Update the output construction around library_names and library_majors to validate that both sequences have equal lengths before calling zip(). Preserve the existing formatting and prevent zip() from silently truncating mismatched inputs without using strict=True.Source: Linters/SAST tools
🤖 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 `@install.sh`:
- Around line 348-351: Remove the lib/stubs glob from the CUDA library probe
loop near the library variable, so stub files cannot count as runtime evidence.
Keep only loader-path locations and require matching-major libcudart, libcublas,
and libcublasLt libraries before selecting a CUDA archive.
- Around line 342-345: Replace the per-library highest-major selection in
cuda_library_major and its callers with logic that collects all detected majors
for libcudart, libcublas, and libcublasLt, then selects the highest major
present in all three libraries and no greater than driver_max. Preserve
rejection when no common supported major exists, and add a regression case
covering libcudart 13/12 with libcublas and libcublasLt 12.
---
Nitpick comments:
In `@scripts/tests/test_install_sh.py`:
- Around line 184-187: Update the output construction around library_names and
library_majors to validate that both sequences have equal lengths before calling
zip(). Preserve the existing formatting and prevent zip() from silently
truncating mismatched inputs without using strict=True.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a04c360-1a15-4989-85fc-002d17536895
📒 Files selected for processing (2)
install.shscripts/tests/test_install_sh.py
| major="$(ldconfig -p 2>/dev/null | grep -oE "${library}\.so\.[0-9]+" | awk -F. '{print $3}' | sort -rn | head -n 1 || true)" | ||
| if [[ -n "$major" ]]; then | ||
| printf '%s\n' "$major" | ||
| return 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Select the highest common CUDA major.
cuda_library_major returns only the highest major for each library. A host with libcudart.so.13 and .12, plus libcublas.so.12 and libcublasLt.so.12, has a complete CUDA 12 lane. This code returns 13, 12, and 12, then rejects the host.
Collect all detected majors. Select the highest major shared by all three libraries that does not exceed driver_max. Add a regression case for this mixed-version host.
Also applies to: 386-396
🤖 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 `@install.sh` around lines 342 - 345, Replace the per-library highest-major
selection in cuda_library_major and its callers with logic that collects all
detected majors for libcudart, libcublas, and libcublasLt, then selects the
highest major present in all three libraries and no greater than driver_max.
Preserve rejection when no common supported major exists, and add a regression
case covering libcudart 13/12 with libcublas and libcublasLt 12.
| for lib in \ | ||
| "$probe_root"/usr/local/cuda*/lib64/"$library".so.* \ | ||
| "$probe_root"/usr/local/cuda*/targets/*/lib/"$library".so.* \ | ||
| "$probe_root"/usr/local/cuda*/targets/*/lib/stubs/"$library".so.*; do |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not treat CUDA stubs as runtime evidence.
Files in lib/stubs are link-time stubs. They do not prove that the dynamic loader can resolve the CUDA runtime libraries. A host can therefore select a CUDA archive that cannot initialize its native runtime.
Remove the lib/stubs probe. Require loader-resolvable matching-major libraries instead.
Based on learnings: Linux host-linked CUDA artifacts require matching-major libcudart, libcublas, and libcublasLt evidence on the dynamic loader path.
🤖 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 `@install.sh` around lines 348 - 351, Remove the lib/stubs glob from the CUDA
library probe loop near the library variable, so stub files cannot count as
runtime evidence. Keep only loader-path locations and require matching-major
libcudart, libcublas, and libcublasLt libraries before selecting a CUDA archive.
Source: Learnings
e455fd0 to
96d54c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/tests/test_ci_lane_workflows.py (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every control-plane matcher branch.
The workflow matcher has rules for
.github/,.omo/,ci/, and several script paths. This test checks only the.github/rule. A later removal of another rule would still pass. Add assertions for each configured rule or execute the matcher with representative paths.Example contract assertions
+ for marker in ( + "filename.startsWith('.omo/')", + "filename.startsWith('ci/')", + "filename === 'scripts/affected-crates.sh'", + "filename === 'scripts/plan-ci.py'", + "filename.startsWith('scripts/plan-')", + "filename === 'scripts/collect-ci-metrics.py'", + "filename === 'scripts/tests/test_plan_ci.py'", + ): + with self.subTest(marker=marker): + self.assertIn(marker, pr_workflow)🤖 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/tests/test_ci_lane_workflows.py` around lines 45 - 51, The test covering the PR workflow currently asserts only the .github/ matcher branch. Extend the assertions in the relevant test method to cover every configured control-plane path rule, including .omo/, ci/, and each script path, or run the matcher against representative paths and verify each is detected.
🤖 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 @.agents/skills/manage-ci/references/current-inventory.md:
- Line 11: Update the pr_builds.yml entry in the workflow inventory table to
explicitly include manual workflow_dispatch runs among the routes handled by
ci-orchestrator.yml, alongside control-plane changes, forks, and migrations.
Preserve the existing description of ordinary same-repository PR routing.
In @.github/workflows/pr_builds.yml:
- Around line 43-57: Update the controlPlaneChanged calculation using the
paginated pull-request file list so it is true when files.length >= 3000, in
addition to the existing filename checks. In the route job permissions, add
pull-requests: read so github.rest.pulls.listFiles can execute successfully.
Apply the same fix in @.github/workflows/pr_builds.yml around lines 43 - 47.
---
Nitpick comments:
In `@scripts/tests/test_ci_lane_workflows.py`:
- Around line 45-51: The test covering the PR workflow currently asserts only
the .github/ matcher branch. Extend the assertions in the relevant test method
to cover every configured control-plane path rule, including .omo/, ci/, and
each script path, or run the matcher against representative paths and verify
each is detected.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a215756-75d9-4258-803e-4651c435dbdd
📒 Files selected for processing (4)
.agents/skills/manage-ci/references/current-inventory.md.github/workflows/pr_builds.ymlci/ci.mdscripts/tests/test_ci_lane_workflows.py
| | Workflow | Trigger | Ownership | | ||
| | --- | --- | --- | | ||
| | `pr_builds.yml` | PR lifecycle, dispatch | Routes same-repository PRs to protected control; bootstraps forks and migrations through `ci-orchestrator.yml` | | ||
| | `pr_builds.yml` | PR lifecycle, dispatch | Routes ordinary same-repository PRs to protected control; bootstraps control-plane changes, forks and migrations through `ci-orchestrator.yml` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document manual dispatch bootstrap routing.
.github/workflows/pr_builds.yml sets bootstrap for every workflow_dispatch at Line 78. This row lists control-plane changes, forks, and migrations, but not manual runs. Add manual runs so the inventory does not imply that dispatches use protected control.
Proposed wording
-| `pr_builds.yml` | PR lifecycle, dispatch | Routes ordinary same-repository PRs to protected control; bootstraps control-plane changes, forks and migrations through `ci-orchestrator.yml` |
+| `pr_builds.yml` | PR lifecycle, dispatch | Routes ordinary same-repository PRs to protected control; bootstraps control-plane changes, forks, migrations and manual runs through `ci-orchestrator.yml` |📝 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.
| | `pr_builds.yml` | PR lifecycle, dispatch | Routes ordinary same-repository PRs to protected control; bootstraps control-plane changes, forks and migrations through `ci-orchestrator.yml` | | |
| | `pr_builds.yml` | PR lifecycle, dispatch | Routes ordinary same-repository PRs to protected control; bootstraps control-plane changes, forks, migrations and manual runs through `ci-orchestrator.yml` | |
🤖 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 @.agents/skills/manage-ci/references/current-inventory.md at line 11, Update
the pr_builds.yml entry in the workflow inventory table to explicitly include
manual workflow_dispatch runs among the routes handled by ci-orchestrator.yml,
alongside control-plane changes, forks, and migrations. Preserve the existing
description of ordinary same-repository PR routing.
7448804 to
25352ab
Compare
1 participant
Summary
scripts/package-release.sh, preserving the published-cuda-13naming.repo-consistency release-targetsin the lightweight PR Quality consistency job and assert that workflow contract.Root cause
PR #1267 removed the legacy bare
-cudainstaller fallback because Linux CUDA releases are published as-cuda-12and-cuda-13. The xtask Orin fixture still inherited ambient CUDA detection from the host, so the immutable Linux release-host runner failed when it had an NVIDIA GPU but no detectable supported CUDA toolkit.Validation
bash -n install.shpython3 -m unittest discover -s scripts/tests -p 'test_install_sh.py'python3 -m unittest discover -s scripts/tests -p 'test_*.py'(409 tests, 7 skipped)cargo fmt --all -- --checkcargo clippy -p xtask --all-targets -- -D warningscargo run -p xtask -- repo-consistency release-targetsactionlint -config-file .github/actionlint.yamlSummary by CodeRabbit
Bug Fixes
Documentation
Tests