Skip to content

Fix #962: Don't perform unnecessary version checks #969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 16, 2025
Merged

Fix #962: Don't perform unnecessary version checks #969

merged 3 commits into from
Sep 16, 2025
+79 −89

Conversation

@mdboom
Copy link
Contributor

@mdboom mdboom commented Sep 15, 2025

Description

closes #962

Checklist

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

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Sep 15, 2025

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@mdboom
Copy link
Contributor Author

mdboom commented Sep 15, 2025

/ok to test

cpcloud
cpcloud reviewed Sep 15, 2025
View reviewed changes
cuda_bindings/cuda/bindings/_internal/cufile_linux.pyx
cdef void* handle = NULL

with gil, __symbol_lock:
driver_ver = get_cuda_version()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is being used below and causing at least some of the compilation failures. Does loading the library actually require computing the version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't actually require it. Just need to change what's generated...

@mdboom
Copy link
Contributor Author

mdboom commented Sep 15, 2025

/ok to test

@leofang leofang added bug Something isn't working cuda.bindings Everything related to the cuda.bindings module P1 Medium priority - Should do labels Sep 15, 2025
@leofang leofang added this to the cuda-python 13-next, 12-next milestone Sep 15, 2025
@github-actions

This comment has been minimized.

Sign in to view
@mdboom
Copy link
Contributor Author

mdboom commented Sep 15, 2025

/ok to test

rwgk
rwgk approved these changes Sep 15, 2025
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

Is there an easy/obvious way to specify libcuda_so = 'libcuda.so.1' in one central place? cuda/bindings/_internal/utils.pyx maybe? Maybe even better, move the entire get_cuda_version() function there? — We could also do that later.

leofang
leofang requested changes Sep 15, 2025
View reviewed changes
cuda_bindings/cuda/bindings/_internal/cufile_linux.pyx
Copy link
Member

Choose a reason for hiding this comment

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

Ralf's comment made me realize... we should also remove get_cuda_version in this file, because it is no longer needed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah... I see now why this is challenging: get_cuda_version is a copy-pasta from the extern pxd. It does not seem that we can remove this locally (i.e. without affecting other teams). I'll let Mike make the final call, I run out of good ideas tonight 🙁

Copy link
Member

Choose a reason for hiding this comment

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

(One silly idea is to split the extern pxd into two, one is always copied/pasted, the other is pasted after the first one only when need_driver is true.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, get_cuda_version is inserted by the binding generator into every set of bindings. I don't see much need to deal with that, though. It's a static function that the compiler removes if not needed.

cuda_bindings/cuda/bindings/_internal/nvjitlink_linux.pyx
Copy link
Member

Choose a reason for hiding this comment

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

ditto

cuda_bindings/cuda/bindings/_internal/nvjitlink_windows.pyx
Copy link
Member

Choose a reason for hiding this comment

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

ditto

cuda_bindings/cuda/bindings/_internal/nvvm_linux.pyx
Copy link
Member

Choose a reason for hiding this comment

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

ditto

cuda_bindings/cuda/bindings/_internal/nvvm_windows.pyx
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@mdboom
Copy link
Contributor Author

mdboom commented Sep 16, 2025

Nice cleanup!

Is there an easy/obvious way to specify libcuda_so = 'libcuda.so.1' in one central place? cuda/bindings/_internal/utils.pyx maybe? Maybe even better, move the entire get_cuda_version() function there? — We could also do that later.

get_cuda_version is needed by other cybind-generated projects, that may not rely on cuda_bindings.

@mdboom mdboom merged commit 9922034 into NVIDIA:main Sep 16, 2025
49 checks passed
@github-actions
Copy link

Doc Preview CI
Preview removed because the pull request was closed or merged.

mdboom added a commit to mdboom/cuda-python that referenced this pull request Sep 19, 2025
@mdboom
Backport NVIDIA#962 and NVIDIA#969 to 12.9.x
72607a5
leofang pushed a commit that referenced this pull request Sep 19, 2025
@mdboom
Backport #962 and #969 to 12.9.x (#992)
553adf7
@cpcloud cpcloud mentioned this pull request Oct 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpcloud cpcloud cpcloud left review comments

@rwgk rwgk rwgk approved these changes

@leofang leofang leofang approved these changes

Assignees

@mdboom mdboom

Labels

bug Something isn't working cuda.bindings Everything related to the cuda.bindings module P1 Medium priority - Should do

Projects

None yet

Milestone

cuda-python 13.0.2 & 12.9.3

Development

Successfully merging this pull request may close these issues.

Avoid unnecessarily loading and checking driver version

4 participants

@mdboom @cpcloud @rwgk @leofang