cuda.bindings: support multiple CTK release lines on main - #2737
Conversation
Moon migration belongs in PR NVIDIA#2659. Restore the pre-Moon selective-CI planner and workflows from 727ef59.
rwgk
added
enhancement
|
/ok to test 09f27fb |
|
/ok to test 7cb81e1 |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Hi @mdboom, I rebuilt the six-layer narrated review branch a third time from the current PR head (7cb81e1):
https://github.com/rwgk/cuda-python/commits/review/multi-ctk-bindings-on-main-20260903-1844-7cb81e1
That branch is only a review aid; this PR remains authoritative. The six commits are meant to be read oldest to newest. Each commit message explains its boundary, suggested review focus, and validation:
- 6abe8bb — import the maintained CUDA 12.9 source root
- dbbdf68 — define the package-root/release-status registry
- 8be614a — plan selective work by package root
- 61e67bb — route builds and tests
- c1bad81 — resolve and validate releases
- 04bfed6 — retire the 12.9 backport workflow
For the shortest useful path, I'd start with layer 2 and then review layers 3-5. Layer 4 is necessarily the broad workflow-integration layer.
I incorporated your latest feedback: ci.tools is now a small package under ci/pyproject.toml; downstream consumers trust normalized package records; pre-commit checks the registry against both bindings packages' metadata; and the generic --package-json behavior was replaced by the purpose-specific write-github-env command.
I then made another simplification pass, removing unsupported planner shapes, unused workflow fallbacks, repeated registry reads, and redundant release metadata/checks while preserving historical-tag compatibility. That pass changed 280 lines in and 412 lines out - a net reduction of 132 lines, including the added focused tests.
The current PR head is fully green. When you have a chance, could you please review this fresh layered view and let me know whether you see anything else blocking this PR? The main decisions I hope to settle are the package-root/release-status model, release routing, and moving CUDA 12.9 maintenance onto main.
| moves to a new toolkit minor; registry validation rejects a configuration | ||
| where the two disagree. | ||
|
|
||
| The Python helpers share registry parsing and validation, so they are modules |
There was a problem hiding this comment.
This implies that (a) you are running from the root of the repo, and (b) you already did a
pip install -r ci/tools/requirements.txt.
requirements.txthas been deprecated for a long time. We should either: add apyproject.tomlforci/toolsand require that the package is installed into a venv (which removes the$PWDproblem), or use PEP 723 comments in each script and not have a proper package altogether.Personally, I think PEP 723 is the simpler solution (it doesn't require juggling virtual environments), but it does require having
uvorpixiaround to run the script.The argument in favor of a package would be if these scripts start to share a lot of logic and helpers between them such that things in ci/tools need to import other things in ci/tools. It doesn't look like they do right now, but in the original mega-PR they started to do that a bit.
Agreed. I chose the package option after the later refactor made the helpers genuinely share logic: bindings_scm_version, check_pixi_cuda_version, check_release_notes, and compute_ci_plan now import the registry/version machinery rather than duplicating it. The PR now adds ci/pyproject.toml, removes both requirements files, and installs ./ci in editable mode at workflow call sites. The documented module invocation therefore works through an installed package and no longer relies on $PWD putting the repository on Python's import path; PEP 723 would repeat the same dependency metadata across several scripts.
| parser.add_argument("--control-config", type=Path) | ||
| commands = parser.add_subparsers(dest="command") | ||
| write_env = commands.add_parser( | ||
| "write-github-env", |
There was a problem hiding this comment.
What's the purpose of this flag? It looks like it just spits the JSON provided right back out, doing nothing to it?
Agreed--it had no useful standalone meaning. --package-json and --github-env are gone. The only JSON-consuming operation is now the purpose-specific write-github-env GITHUB_ENV subcommand, which reads a selected package record from stdin, validates it, and appends only the supported environment variables. This removes the round-trip behavior and prevents that operation from being accidentally combined with registry-selection modes.
| types: [text] | ||
|
|
||
| - id: check-bindings-config | ||
| name: Check bindings registry tracks package metadata |
There was a problem hiding this comment.
This duplication from
versions.ymlis worrisome, but I don't see any way around it. I think a precommit hook to make sure it's in sync withversions.ymlwould be a good idea.
Agreed. I added this check-bindings-config hook, triggered by ci/versions.yml or any bindings pyproject.toml. Registry validation reads each package root's tag_regex, constructs v<toolkit_version> from the registry, and requires the SCM regex to accept that exact version. Focused SCM tests additionally verify that the current 13.3 pattern rejects 13.4 and that metapackage SCM metadata matches both bindings roots. A normal release-family update on either side now fails locally and in CI if the duplicated metadata drifts.
kkraus14
left a comment
kkraus14
left a comment
There was a problem hiding this comment.
Codex review at 7cb81e114f8f4fabe3e4fa35a9f2bb5c11b5a45a. I left inline comments for the code findings and temporary-ignore cleanup. Two branch-wide blockers do not have a meaningful single-line anchor: this head currently conflicts with main in the backport/workflow files, so it needs to be updated and CI rerun; and I could not find a CI: Release 12.9 dry run for this head, so the maintenance release path is not yet validated end to end. The successful normal CI run rebuilt both bindings lines because CI files changed, so it does not cover the mixed selective-build case called out inline. The pipeline-diagram follow-up is already covered by the existing review thread and #2740.
| - name: Download cuda.bindings build artifacts | ||
| if: ${{ (env.TEST_BINDINGS == 'true' || env.TEST_CORE == 'true' || env.TEST_PYTHON == 'true') && | ||
| env.BINDINGS_SOURCE == 'main' }} | ||
| env.BINDINGS_SOURCE == 'local' }} |
There was a problem hiding this comment.
Codex review — High: this gate consumes the aggregate TEST_BINDINGS / TEST_CORE / TEST_PYTHON flags for every CUDA-major matrix row. For a mixed change such as a v12 bindings file plus a cuda_core test, the plan builds only the v12 bindings test artifact but schedules core tests for both v12 and v13; the v13 row then reaches this step and requests a v13 artifact that was never built. The Windows workflow has the same shape. Please derive these gates per selected bindings line (or otherwise ensure each local row only downloads an artifact its root built) and add a mixed-change regression test.
| ) | ||
|
|
||
| missing_distributions = sorted(expected_distributions - set(seen_versions)) | ||
| missing_distributions = sorted(expected_distributions - seen_distributions) |
There was a problem hiding this comment.
Codex review — High: this verifies only that each distribution name appeared. I reproduced a successful v12 validation with a directory containing just one correctly named wheel, even though a release should contain every supported Python/platform/ABI variant. That can allow a partial TestPyPI/PyPI release after an artifact is missing, deleted, or expired. Please validate the complete expected wheel matrix (or exact expected artifact set from the build plan) before upload and add a partial-matrix rejection test.
| else | ||
| echo "error: cannot find ci/versions.yml or ci/versions.json" >&2 | ||
| exit 1 | ||
| resolved_bindings_package=$(cd .ci-control && python3 -m ci.tools.bindings_config \ |
There was a problem hiding this comment.
Codex review — Medium: cuda-core-v* releases reach this fallback because release.yml does not pass bindings-package for cuda-core and the branch above recognizes only bare v* tags. This selects current control-plane bindings metadata instead of the metadata in the release tag. A delayed or historical core release can therefore request a newer bindings artifact that its tagged CI run never built (for example, cuda-core v1.0.0 used CUDA 13.2.1 while current control uses 13.3.0). Please resolve the bindings dependency from the checked-out release tag registry for component-prefixed releases too.
| if not release_source_root.is_dir(): | ||
| raise BindingsConfigError(f"release source root is not a directory: {release_source_root}") | ||
|
|
||
| tagged_config_path = release_source_root / "ci" / "versions.yml" |
There was a problem hiding this comment.
Codex review — Medium: real v12.9.1–v12.9.7 and v13.0.x tags store this metadata in ci/versions.json, not ci/versions.yml. Treating the YAML as absent sends those tags to the legacy fallback, which derives the line from today's control config; that only works while today's pins happen to match. Please probe and parse both legacy filenames from the tag tree before using control metadata, with a fixture shaped like an actual old tag.
| tag_regex = "^(?P<version>v\\d+\\.\\d+\\.\\d+(?:[ab]\\d+)?)" | ||
| git_describe_command = ["git", "describe", "--dirty", "--tags", "--long", "--match", "v*[0-9]*"] | ||
| # Accept the project's prerelease, post-release, and development suffixes. | ||
| tag_regex = "^(?P<version>v13\\.3\\.\\d+(?:(?:a|b|rc)\\d+)?(?:\\.post\\d+)?(?:\\.dev\\d+)?)$" |
There was a problem hiding this comment.
Codex review — Medium: accepting .devN here makes a development tag valid for tag CI, but cuda_bindings/docs/build_docs.sh truncates the version to the first three dot components. As a result, v13.3.0.dev1 deploys production docs under 13.3.0, potentially occupying or overwriting the stable path even though release-wheel validation rejects development releases. Please retain the dev suffix in the docs path or prevent production docs deployment for dev tags.
| ## Installing from PyPI | ||
|
|
||
| ```console | ||
| $ pip install -U cuda-python |
There was a problem hiding this comment.
Codex review — Medium: this is the CUDA 12 maintenance documentation, but the unpinned metapackage command now resolves the current v13 line. The [all] and Conda examples have the same issue, and the copied README/DESCRIPTION links target /cuda-bindings/latest/ (v13). Please pin the maintenance line (for example, cuda-python~=12.9.0 or cuda-bindings~=12.9.0) and direct readers to the versioned 12.9 docs or version selector.
| cuda_devices = get_cuda_device_names() | ||
| nvml_devices = get_nvml_device_names() | ||
|
|
||
| assert cuda_devices == nvml_devices, "CUDA and NVML device lists do not match" |
There was a problem hiding this comment.
Codex review — Medium: this copied v12 test omits the fixes already present in the current-root test for Orin/Thor and CUDA_VISIBLE_DEVICES. On a multi-GPU host with CUDA_VISIBLE_DEVICES=0, CUDA reports one visible GPU while NVML reports all GPUs, so this exact equality fails. Please port the current-root handling to the v12 copy, consistent with the cross-root handwritten-fix policy in cuda_bindings_12/MAINTENANCE.md.
| git(tmp_path, "init") | ||
| git(tmp_path, "config", "user.name", "CUDA Python CI") | ||
| git(tmp_path, "config", "user.email", "cuda-python@nvidia.com") | ||
| git(tmp_path, "config", "commit.gpgsign", "false") |
There was a problem hiding this comment.
Codex review — Low: this temporary repository disables commit signing but still inherits global tag signing. With tag.gpgSign=true, the plain git tag calls fail or prompt (I reproduced 14 failures); with tag signing disabled, the same suite passes. Please set tag.gpgSign=false here and in the analogous temp-repo helper in test_lookup_run_id.py, or create tags with --no-sign, so the tests are hermetic.
| @@ -0,0 +1,7 @@ | |||
| # REMOVE THIS FILE BEFORE MERGING THIS PR | |||
There was a problem hiding this comment.
Codex review — Pre-merge cleanup: this temporary file is still tracked and suppresses link checks for paths that do not yet exist on main. Please remove it before the final CI run and merge, as its header and the PR checklist require.
| # run-id blank so automatic tag-run discovery, artifact filtering, and version | ||
| # validation are exercised. | ||
|
|
||
| on: |
There was a problem hiding this comment.
I find it hard to answer this from reading alone:
In this new model, 13.x and 12.x will be released from the same commit, right? And the same commit would be tagged with both v13.4.2 and v12.9.9. That's all supported by git, but how does this infrastructure handle that correctly, since it would have to dispatch based on the commit.
|
I converted to Draft to have direct control over triggering the CI. |
# Conflicts: # .github/workflows/backport.yml # .github/workflows/build-wheel.yml # .github/workflows/ci-pixi-source-test.yml # .github/workflows/ci.yml # .github/workflows/coverage.yml # .github/workflows/release-upload.yml # .github/workflows/release.yml # .github/workflows/test-wheel-linux.yml # .github/workflows/test-wheel-windows.yml # ci/tools/env-vars # ci/tools/run-tests # ci/versions.yml # cuda_python/setup.py
|
/ok to test 98c987a |
# Conflicts: # ci/tools/env-vars
|
/ok to test 8e1822c |
|
/ok to test cb95f0d |
3 participants
REMINDER
After approval and before triggering the final merge CI, remove the temporary
.lycheeignore. It excludes only three canonicalmain/cuda_bindings_12URLs that cannot resolve until this PR is merged. The authored-sourcelycheepre-commit hook is explicitly skipped by the CI pre-commit jobs; CI separately checks links in rendered documentation. Removing the file therefore will not introduce a required-check failure before merge.After merging, run
pre-commit run lychee --all-fileson freshmainto validate those links.Summary
Closes #1199.
This is the writable continuation of Keith Kraus's original PR #2675, "cuda.bindings: build 12.9 and 13.x selectively from main". Keith authored most of the CUDA 12 source import and the initial build, test, and release integration. GitHub closed #2675 automatically when its temporary base branch was deleted after #2467 merged; this replacement preserves that work and incorporates the subsequent review-driven redesign and hardening.
This PR moves active development of both released CUDA bindings lines onto
main:cuda_bindings_12/maintenancecuda_bindings/currentThe two roots are alternative source trees for the same
cuda-bindingsdistribution andcuda.bindingsimport namespace. Either can be built and installed independently, but they are not separately named and are not intended to coexist in one environment. CI, documentation, and release workflows select the appropriate source root and toolkit pin.After merge,
mainbecomes the active source of truth for both lines. The historical12.9.xbranch remains only as a read-only release record rather than an active backport or artifact-source branch.What Changes
cuda_bindings_12/, including packaging, tests, examples, documentation, and its lock file.cuda.build/cuda.prev_buildfields andbackport_branchsetting with a package-root registry inci/versions.yml. Each unique root declares its exact toolkit pin and scalarcurrentormaintenancerelease status.12.9.xbackport workflow and branch-sourced artifact flow.cuda_bindings_12/MAINTENANCE.md.Review Guide
Because the raw PR is dominated by the CUDA 12 tree (approximately 98% of the additions), the following six-commit branch decomposes the architecture at review snapshot
7cb81e1:review/multi-ctk-bindings-on-main-20260903-1844-7cb81e1
Read it oldest to newest. Each commit message explains its boundary, suggested review focus, and final-tree validation:
The highest-value review path is layers 2-5. Layer 1 is predominantly Keith's imported CUDA 12.9 tree; layer 6 isolates the branch-ownership policy. These are narrative review layers, not independently deployable states, and the PR branch remains authoritative.
Post-snapshot changes are best reviewed in these groups:
main, and refresh the resulting CUDA Core benchmark lock.setuptools_scm.Design
ci/versions.ymlmaps each unique root directly to its toolkit pin and release status. There are no synthetic line IDs or separate role-to-line mappings.[tool.setuptools_scm].tag_regexdefines its accepted release family. Registry validation checks that this metadata agrees with the configured toolkit pin and derives the CTK target and CUDA ABI fields.currentroot and onemaintenanceroot with different CUDA ABI majors. Other shapes fail rather than implying workflow support that does not exist.ci.toolspackage centralizes registry, version, planner, and release logic. Workflows consume explicit normalized inputs rather than reconstructing bindings identity from directory names or hard-coded CUDA majors.ci/versions.ymlor legacyci/versions.jsonfrom the tag tree and handles older tags that predatesetuptools_scm. Malformed modern registries and unsupported tag forms fail closed.Reviewer Decisions
Please explicitly accept or reject these policies:
mainis the sole active source of truth. The historical12.9.xbranch receives no further routine or emergency backports. CUDA 12 fixes are made incuda_bindings_12/onmain, alongside a corresponding current-root change when applicable.release_statusrecords its place in the release lifecycle without introducing a second line identity.Validation
Validation completed successfully for final head
cb95f0d73e1:pre-commit run --all-filespassed, including Ruff, actionlint, mypy, YAML/TOML/RST checks, generated-file seals, SCM/registry checks, and authored-sourcelycheewith the three temporary exact exclusions described in the REMINDER.setuptools_scmtags, component-prefixed releases, complete binary wheel matrices, and partial-matrix rejection. Local validation against the actualv12.9.7andv13.4.1tag trees derived 21 and 23 required wheel targets per binary distribution respectively.View all PR checks, the completed full CI run, the selected-source smoke run, and the Pixi lockfile freshness run.
Remaining Before Merge
CI: Releasedry run for this head. The historical v12.9.7 run succeeded, but its artifacts have expired; normal PR artifacts cannot substitute for a fresh tag-shaped run because release lookup deliberately validates the tag event, ref, SHA, and release-version wheels..lycheeignoreand run the final merge CI as described in the REMINDER.Out of Scope