Skip to content

test(cuda.bindings): cover sm_90a, sm_103 and sm_103a in the nvJitLink and nvFatbin tests - #3218

Closed
0z5a wants to merge 2 commits into
NVIDIA:mainfrom
0z5a:test/nvjitlink-nvfatbin-sm90a-sm103
Closed

0z5a wants to merge 2 commits into
NVIDIA:mainfrom
0z5a:test/nvjitlink-nvfatbin-sm90a-sm103

Conversation

@0z5a

@0z5a 0z5a commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Description

Closes #3217.

The canonical nvJitLink and nvFatbin tests now cover sm_90a, sm_103, and sm_103a. The nvJitLink parameters pair them with the minimum PTX versions 8.0, 8.8, and 8.8; the nvFatbin tests cross each architecture with the existing independent PTX version list. In response to review, the legacy test additions were removed.

The original parametrization was written with Claude Opus 5.5 and reviewed by the contributor. This PR changes existing parameter lists and adds no test function.

Testing

  • On B300 and GH200, the original revision ran the four canonical and legacy files: 186 passed before, 282 passed after. These historical counts include the legacy additions that have since been removed; they do not describe the current head.
  • On each machine, a separate script (outside this PR) built and loaded sm_103a or sm_90a images using nvJitLink, NVRTC and nvFatbin, launched a kernel and checked the result.
  • At the review follow-up head: Ruff and Python syntax compilation passed for the two canonical test files, and git diff --check passed. Native CUDA tests have not been rerun on this head.

The historical runs used cuda-python 2da9c65a, CUDA 13.4.2 and Python 3.12 on B300 (driver 610.57.04) and GH200 (driver 595.58.03).

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes (no documentation is affected).

…k and nvFatbin tests

The four architecture lists stopped at sm_100 and had no arch-specific target, so
nothing exercised the "a" targets or sm_103 (B300 / GB300), where nvJitLink no longer
serves sm_103 from sm_100 code (the reason the RDC fixture moved to -arch=all in NVIDIA#2296).

Add sm_90a, sm_103 and sm_103a. In the nvJitLink tests the PTX versions pair with the
architectures, so the new entries use each target's minimum PTX ISA version: 8.0 for
sm_90a and 8.8 for sm_103 / sm_103a. The nvFatbin tests take the PTX version as an
independent parameter, so their version list is unchanged.

Tests written with Claude Opus 5.5 and reviewed by the contributor.

Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
@copy-pr-bot

copy-pr-bot Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the cuda.bindings Everything related to the cuda.bindings module label Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Summary

Summary by CodeRabbit

  • Tests
    • Expanded CUDA fatbin and JIT linking test coverage to include sm_90a, sm_103, and sm_103a architectures.
    • Added corresponding PTX version combinations to JIT linking tests, helping validate behavior across a broader range of target architectures.

The nvFatbin and nvJitLink test parameter lists now include sm_90a, sm_103, and sm_103a. The nvJitLink lists pair those targets with PTX versions 8.0, 8.8, and 8.8, respectively.

Changes: Architecture test coverage

Layer / File(s) Summary
nvFatbin architecture parameters
cuda_bindings_13/tests/test_nvfatbin.py, cuda_bindings_13/tests/legacy_api/test_legacy_nvfatbin.py
The parameter lists add sm_90a, sm_103, and sm_103a.
nvJitLink architecture and PTX pairs
cuda_bindings_13/tests/test_nvjitlink.py, cuda_bindings_13/tests/legacy_api/test_legacy_nvjitlink.py
The parameter lists add sm_90a/8.0, sm_103/8.8, and sm_103a/8.8 pairs.

Assessment against linked issues

Objective Addressed Explanation
Add sm_90a, sm_103, and sm_103a to the nvFatbin and nvJitLink test lists, with the requested nvJitLink PTX versions [#3217] ✅

Suggested labels: test, cuda.bindings

Suggested reviewers: rwgk

Priority: Low

Change: Other

Merge Risk: Low · previous review 250e5

The new architecture cases should be removed from the legacy suites or the repository exception should be explicitly accepted before merging.

Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a2fb804 and 250e500.

Files selected for processing: 4
  • cuda_bindings_13/tests/legacy_api/test_legacy_nvfatbin.py
  • cuda_bindings_13/tests/legacy_api/test_legacy_nvjitlink.py
  • cuda_bindings_13/tests/test_nvfatbin.py
  • cuda_bindings_13/tests/test_nvjitlink.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

@Andy-Jost
Andy-Jost self-requested a review October 27, 2026 22:51
@Andy-Jost

Copy link
Copy Markdown
Contributor

/ok to test 250e500

@github-actions

Copy link
Copy Markdown
Contributor

@Andy-Jost Andy-Jost self-assigned this Oct 28, 2026

@Andy-Jost Andy-Jost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the new entries check out. The PTX pairs are right (8.0 for sm_90a, 8.8 for sm_103 and sm_103a), an offline probe against the 13.4 nvJitLink, nvFatbin, NVRTC, and nvcc accepted every new combination, and the +96 case count matches the parametrization. In CI these files run only on the CUDA 13.4.2 rows, so no per-architecture skip is needed.

One change before merge: the two tests/legacy_api hunks. cuda_bindings_13/AGENTS.md and the legacy README say those files are not to be added to, only fixed when a test breaks. The canonical test_nvjitlink.py and test_nvfatbin.py carry the new coverage, so please drop the changes to test_legacy_nvjitlink.py and test_legacy_nvfatbin.py. CodeRabbit made the same point on both files.

The failing aarch64 3.15t job is test_coredump_bool_uses_single_byte_storage, which this PR does not touch; I'm re-running it.

@Andy-Jost Andy-Jost added enhancement Any code-related improvements P2 Low priority - Nice to have labels Oct 28, 2026
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
@leofang

leofang commented Oct 30, 2026

Copy link
Copy Markdown
Member

Sorry I hate to say this, but this does not add value to our test suite TBH. The binding tests are meant to examine if we are passing the Python arguments to the underlying C APIs correctly, not meant to be exhaustive or offer full coverage (which is the job of the respective libraries). Next time, please make sure you discuss first with us per the contributor guide before sending a PR.

@0z5a

0z5a commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the explanation, that makes sense. Since the binding tests are meant to check argument passing rather than library coverage, I am closing this. I will open an issue to coordinate before sending similar PRs in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.bindings Everything related to the cuda.bindings module enhancement Any code-related improvements P2 Low priority - Nice to have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA]: Test nvJitLink and nvFatbin against sm_90a, sm_103 and sm_103a

3 participants