-
Notifications
You must be signed in to change notification settings - Fork 214
Cythonize cuda.core
#1041
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.
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
Cythonize cuda.core
#1041
Conversation
/ok to test 277a5b7 |
277a5b7
to
0b945cb
Compare
/ok to test 0b945cb |
0b945cb
to
8a636e5
Compare
/ok to test 8a636e5 |
/ok to test fbe9325 |
fbe9325
to
d3c7877
Compare
/ok to test d3c7877 |
d3c7877
to
a59e7c0
Compare
/ok to test a59e7c0 |
a59e7c0
to
37fe7eb
Compare
/ok to test 37fe7eb |
37fe7eb
to
9e799e4
Compare
/ok to test 473f8f2 |
@leofang given the scope of this change, I think we should consider punting this until after the 0.4.0 release. That way we can kick the tires a bit on it in development before releasing it outwards? |
What would be the concern(s) that we must address before merging? I'd rather punt on pushing out 0.4.0 by a few more days instead of punting on this PR. It's an arbitrary timeline that we set to ourselves, and it seems even without considering this PR we have other P0 that could potentially also be slipping. |
There is yet another merge conflict... We need to enforce the practice that only P0 PRs can be merged on the week of release 🤬 |
I'll review/merge #1020 first, and then solve the merge conflicts at once. |
If you feel confident we can always roll it back if it turns out to be an issue, but that sounds like a potentially painful 0.4.1. The biggest concern that I have is that we now have two different wheel builds and only the combined version is tested in CI. Local users building from source will be using a package that isn't tested in CI. |
Ah! I guess the approach of the current PR is based on the implicit assumption that I've been sticking to... that we optimize over end user experience over developer experience 😅 I think we will find issues during daily development if it does not work, because the expectation is that locally one would only build against a single CUDA version. But if adding an CI can help ensuring this, I can add a separate CI pipeline in the next PR? (It'd have to be a standalone pipeline, though, no participation in the build/test matrices.) |
Of course we optimize for the end user, but if we accidentally break being able to build locally because its uncovered in CI that will inevitably impact the quality of software we deliver. Adding a separate CI pipeline sounds reasonable and doing it in a follow up is fine. Ideally we cover both CUDA 12.x and 13.x on both Windows and Linux? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor suggestions. (This isn't a real review, I only looked through top to bottom for general background.)
from cuda.core.experimental._graph import ( | ||
try: | ||
import cuda.bindings | ||
except ImportError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ModuleNotFoundError
be better here (less likely to mask bugs)?
(Also below, current line 21)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've been catching ImportError just fine and the linter does not complain, I suppose it is OK.
# Import all symbols from the module | ||
globals().update(versioned_mod.__dict__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be moved into the else:
branch a few lines down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can, but my slight preference is to keep everything in the try
block to make the intention clear, and use the else
block to only do cleanup (to keep the main namespace tidy).
/ok to test 4a37f97 |
|
Description
closes #866
related to #846.
This PR implements the infrastructure so that all
cuda.core
modules can be properly cythonized and that accessing thecuda.bindings
Cython layer (from cuda.bindings.cyXXXXX cimport ...
) is safe. For local development, nothing is changed except for needing to specifyCUDA_PATH
, same as when buildingcuda.bindings
.The changes involve:
cuda-bindings
version to the build-isolation environment, prior to firing up the Cython compiler.cu12
/cu13
extension modules is done at the import time.cuda.core
wheels against two CUDA major versions, and use a script to merge the payloads. Only the final wheel is stored and used in the test stage (and can be released as before using the release workflow); thecu12
/cu13
wheels are thrown away after the merge.PIP_FIND_LINKS
is the key to allow building against the local, unreleasedcuda.bindings
wheelsChecklist