Skip to content

Add more ObjectCode constructors #652

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

Merged
merged 9 commits into from
May 29, 2025

Conversation

brandon-b-miller
Copy link
Contributor

This PR adds the missing ObjectCode ctors needed for NVIDIA/numba-cuda#133.

Closes #629

Copy link
Contributor

copy-pr-bot bot commented May 22, 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.

@kkraus14
Copy link
Collaborator

Can we add tests for these new constructors?

@brandon-b-miller
Copy link
Contributor Author

Can we add tests for these new constructors?

Right, good question, I was going to ask about this. It's not quite as straightforward to follow the pattern in #470 because this relies on Program being able to compile the original CUDA source to all of the flavors of input we want to test.

We might need to compile some files in the test environment to give us some fatbins, ltoIR objects, etc to test. My first idea was maybe to take inspiration from numba-cuda's binary generation makefile and vendor a portion/all of it here. We'd want to think carefully about what library should test what though to avoid duplicating things and share as many components as possible even across tests, What do you think @kkraus14 ?

@leofang
Copy link
Member

leofang commented May 22, 2025

Vendoring Numba's existing test (with modification) seems to be a good idea, but @brandon-b-miller could you confirm that if cuda.core adds this change and numba-cuda integrates with it, numba-cuda tests can pass? If so I feel comfortable to merge as is and revisit the tests in a next PR.

@leofang leofang self-requested a review May 22, 2025 13:56
@leofang leofang added this to the cuda.core beta 4 milestone May 22, 2025
@brandon-b-miller
Copy link
Contributor Author

Vendoring Numba's existing test (with modification) seems to be a good idea, but @brandon-b-miller could you confirm that if cuda.core adds this change and numba-cuda integrates with it, numba-cuda tests can pass? If so I feel comfortable to merge as is and revisit the tests in a next PR.

This commit:

update for anticipated cuda-python changes

Implements the corresponding numba-cuda changes. Doing this reveals the need for #526, as it seems that is needed for numba's caching machinery. Other then that, the commit passes tests for me locally using CUDA 12, though I'd still like to test in an MVC scenario.

@leofang
Copy link
Member

leofang commented May 22, 2025

Sounds good, please feel free to take over #526. btw just for completeness, could you also add support for CU_JIT_INPUT_LIBRARY to below line 87?

@leofang
Copy link
Member

leofang commented May 22, 2025

Two other test ideas:

  • For fatbins, we could consider adding the nvfatbin bindings and use it to generate fatbins in memory (Add nvfatbin bindings #156)
  • For libraries/objects, we could consider the approach taken by @rwgk and embed a binary blob in the test suite (example) and check in the reproducing steps or scripts in case we need to regenerate the blob (example)

@leofang leofang added P0 High priority - Must do! feature New feature or request cuda.core Everything related to the cuda.core module labels May 23, 2025
@brandon-b-miller
Copy link
Contributor Author

@leofang do you still want to move forward with merging this as-is? I confirmed numba-cuda tests pass with these changes FWIW, and I am happy to write up/take on a separate issue for developing a native set of explicit tests.

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.

Small nit.

@leofang
Copy link
Member

leofang commented May 28, 2025

and I am happy to write up/take on a separate issue for developing a native set of explicit tests

I am supportive for addressing native tests in a separate PR.

@brandon-b-miller
Copy link
Contributor Author

Raised #663

leofang
leofang previously approved these changes May 29, 2025
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

/ok to test eb2bc8b

@leofang
Copy link
Member

leofang commented May 29, 2025

/ok to test eb2bc8b

@rwgk
Copy link
Collaborator

rwgk commented May 29, 2025

/ok to test

This comment has been minimized.

@rwgk rwgk merged commit 5267a41 into NVIDIA:main May 29, 2025
102 of 103 checks passed
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module feature New feature or request P0 High priority - Must do!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more alternative constructors to ObjectCode
4 participants