Fix #789: Remove cyclical import between driver and _lib.utils

[mdboom]

Opening as a draft PR because I'm not sure this is the only option we have, but I'd like to get some feedback and CI anyway.

This basically just moves _lib.utils into driver, which isn't so bad. It does greatly simplify the generated Cython code.

This turns _lib.utils into something that is textually included from .pxd and .pyx files, rather than imported.

closes

Checklist

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

[copy-pr-bot]

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

Contributors can view more details about this message here.

[mdboom]

/ok to test

[Copilot]

Pull Request Overview

This pull request removes a cyclical import between the driver and _lib.utils modules by consolidating the utils module content directly into the driver module. This simplifies the build process and Cython code generation while maintaining the same functionality.

• Moves helper classes and utilities from _lib.utils into the driver module
• Updates all references from utils.HelperX to driver.HelperX across runtime and nvrtc modules
• Modifies the setup.py to handle the new module structure during compilation

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
cuda_bindings/setup.py	Adjusts build configuration to handle param_packer.cpp dependency for driver.pyx only
cuda_bindings/cuda/bindings/runtime.pyx.in	Updates all utils references to use driver module instead
cuda_bindings/cuda/bindings/runtime.pxd.in	Removes utils import and updates helper class references
cuda_bindings/cuda/bindings/nvrtc.pyx.in	Updates utils references to use driver module and adds driver import
cuda_bindings/cuda/bindings/nvrtc.pxd.in	Removes unused utils import
cuda_bindings/cuda/bindings/driver.pyx.in	Adds complete utils functionality including helper classes and imports param_packer
cuda_bindings/cuda/bindings/driver.pxd.in	Includes helper class declarations from utils and adds necessary imports

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

[nitpick] The comment indicates this code is 'From utils.pyx.in' but the section ends with '#### <- From utils.pyx.in' at line 672, creating inconsistent comment formatting. Consider using consistent markers like '#### Begin utils.pyx.in content' and '#### End utils.pyx.in content'.

[Copilot]

[nitpick] Inconsistent comment formatting with the opening marker at line 10. The opening uses '#### from utils.pyd.in' (with incorrect extension) while closing uses '#### <-- from utils.pxd.in'. Consider standardizing both markers.

[mdboom]

This does create a nice, clean import tree:

[kkraus14]

@mdboom another avenue that could possibly work here is to drop the _lib.utils.pxd file and move the _lib.utils.pyx.in --> _lib.utils.pxi.in. https://cython.readthedocs.io/en/latest/src/userguide/faq.html#what-is-the-difference-between-a-pxd-and-pxi-file-when-should-either-be-used

This would allow us to effectively textually inline all of the functionality of _lib.utils into each module and avoid needing to cimport that functionality at all. It obviously would duplicate code and increase binary size a bit, but it would reduce our ABI surface area as well since we wouldn't need to declare these functions in the .pxd files as well.

The other thing is that I think we still need to keep the _lib.utils.pyx file (and possibly the _lib.utils.pxd file?) since we can't break the ABI.

[mdboom]

@mdboom another avenue that could possibly work here is to drop the _lib.utils.pxd file and move the _lib.utils.pyx.in --> _lib.utils.pxi.in. https://cython.readthedocs.io/en/latest/src/userguide/faq.html#what-is-the-difference-between-a-pxd-and-pxi-file-when-should-either-be-used

Good suggestion. I was afraid of the "multiple copies of the same class" problem, where driver._lib.utils.Helper would be different from runtime._lib.utils.Helper. It is, but it doesn't seem to matter in this case since these classes are internal and used briefly with no isinstance etc. where that would matter.

This would allow us to effectively textually inline all of the functionality of _lib.utils into each module and avoid needing to cimport that functionality at all. It obviously would duplicate code and increase binary size a bit, but it would reduce our ABI surface area as well since we wouldn't need to declare these functions in the .pxd files as well.

I think we still need to declare these classes in the .pxd, since there are classes in drivers.pxd with members that are declared in _lib/utils.pxd. That said, I don't think this makes the ABI surface any /worse/.

The other thing is that I think we still need to keep the _lib.utils.pyx file (and possibly the _lib.utils.pxd file?) since we can't break the ABI.

Strictly speaking, yes. If we need it, we should add some testing since it would no longer be imported in the normal course of things and our test suite would no longer exercise it. (I ask this as a n00b) -- does ABI matter for strictly private modules like this?

[mdboom]

/ok to test

[leofang]

The other thing is that I think we still need to keep the _lib.utils.pyx file (and possibly the _lib.utils.pxd file?) since we can't break the ABI.

Strictly speaking, yes. If we need it, we should add some testing since it would no longer be imported in the normal course of things and our test suite would no longer exercise it. (I ask this as a n00b) -- does ABI matter for strictly private modules like this?

I would not concern with private modules' API/ABI. They should neither be import'd nor cimport'd following the normal Python convention.

I think we still need to declare these classes in the .pxd, since there are classes in drivers.pxd with members that are declared in _lib/utils.pxd. That said, I don't think this makes the ABI surface any /worse/.

This seems weird to me. If we have cdef classes using these members, we should just do literal include of the .pxi file in the .pxd files defining these classes. A .pxi shouldn't need an accompanying .pxd.

[kkraus14]

I would not concern with private modules' API/ABI. They should neither be import'd nor cimport'd following the normal Python convention.

I believe _lib.utils was cimported in driver.pxd / runtime.pxd / nvrtc.pxd which is why there's the ABI concern.

[mdboom]

I would not concern with private modules' API/ABI. They should neither be import'd nor cimport'd following the normal Python convention.

I believe _lib.utils was cimported in driver.pxd / runtime.pxd / nvrtc.pxd which is why there's the ABI concern.

Yeah, that makes sense. This will definitely change the ABI then. Particularly, all of utils' classes will change from _lib.utils.X to _X (and I don't think Cython provides a clean way to maintain the old one, but I'll see what I can find...)

[mdboom]

I think we still need to declare these classes in the .pxd, since there are classes in drivers.pxd with members that are declared in _lib/utils.pxd. That said, I don't think this makes the ABI surface any /worse/.

This seems weird to me. If we have cdef classes using these members, we should just do literal include of the .pxi file in the .pxd files defining these classes. A .pxi shouldn't need an accompanying .pxd.

The problem is that the types in utils are exposed as members of the types in driver.pxd. So we definitely need both definitions and implementation, one included from the pxd and one from the pyx.

[mdboom]

/ok to test

[github-actions]

Doc Preview CI
🚀 View preview at https://nvidia.github.io/cuda-python/pr-preview/pr-865/
https://nvidia.github.io/cuda-python/pr-preview/pr-865/cuda-core/
https://nvidia.github.io/cuda-python/pr-preview/pr-865/cuda-bindings/
Preview will be ready when the GitHub Pages deployment is complete.

[mdboom]

/ok to test

[mdboom]

I have (1) made the util classes "private" (_-prefixed) and (2) resolved the problem with compiling param_packer.cpp from multiple threads.

So that does leave the ABI issue. I'm a little unclear about what this means in a Cython context and what we should be guaranteeing.

I ran the changes through the C-level abi-compliance-checker, and I think we are fine at a C ABI compliance level. The reordering of fields in the __pyx_mstatetype struct would be worrying, but IIUC Cython modules don't dig directly into another module's state, they do (Python) lookups on the module's namespace. And in any event, these private fields were never exposed at the Python level anyway.

[copy-pr-bot]

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]

So that does leave the ABI issue. I'm a little unclear about what this means in a Cython context and what we should be guaranteeing.

I'm unclear on the implementation details of cimporting, but the main thing we want to ensure is that if someone built something against a previous minor version of cuda.bindings and they used both Python and Cython functions that if they upgrade to a new minor version of cuda.bindings that they don't get missing symbols or imports or segfaults or other errors at runtime due to changes in the generated .so files.

[mdboom]

So that does leave the ABI issue. I'm a little unclear about what this means in a Cython context and what we should be guaranteeing.

I'm unclear on the implementation details of cimporting, but the main thing we want to ensure is that if someone built something against a previous minor version of cuda.bindings and they used both Python and Cython functions that if they upgrade to a new minor version of cuda.bindings that they don't get missing symbols or imports or segfaults or other errors at runtime due to changes in the generated .so files.

Got it. I think at this point I don't know -- probably the only way to find out is to try it.

[mdboom]

So that does leave the ABI issue. I'm a little unclear about what this means in a Cython context and what we should be guaranteeing.

I'm unclear on the implementation details of cimporting, but the main thing we want to ensure is that if someone built something against a previous minor version of cuda.bindings and they used both Python and Cython functions that if they upgrade to a new minor version of cuda.bindings that they don't get missing symbols or imports or segfaults or other errors at runtime due to changes in the generated .so files.

Got it. I think at this point I don't know -- probably the only way to find out is to try it.

For an experiment, I built cuda_core against main, and installed it along cuda_bindings built with this PR. (cuda_core cimports some things from cuda.bindings.driver). The cuda_core test suite passes. I /think/ that's sufficient proof that we don't have ABI issues, but lmk if that still may be hiding something we need to confirm works.

I do think we should remove the _lib.utils module -- it was always private, is no longer used in the course of normal use, and has its API changed by this PR (all the symbols now have a _ prefix). Nobody should have been importing and using it directly -- but if this is an xkcd 1172 situation, let me know. If we keep it, it will need to compile without the _ prefixes in that context and add testing so it doesn't bitrot.

[mdboom]

/ok to test

[mdboom]

/ok to test

[leofang]

Thanks for looking into it @mdboom!

For an experiment, I built cuda_core against main, and installed it along cuda_bindings built with this PR. (cuda_core cimports some things from cuda.bindings.driver). The cuda_core test suite passes. I /think/ that's sufficient proof that we don't have ABI issues, but lmk if that still may be hiding something we need to confirm works.

two notes here:

1. I think this should be the other way around: We build a project against older cuda.bindings (say 13.0.1), and run it with newer cuda.bindings (from this PR) installed in the venv. Xref: Building using versions of cuda-python with the new layout breaks runtime compatibility with older versions #274 (comment)
2. cuda-core is not a good testbed because it currently does not cimport cuda.bindings (by design; I see you asked a question on this in the other issue, I'll expand my answer there). Either RAPIDS/rmm or a simple Cython module like these should be sufficient to detect at least import issues

[mdboom]

Thanks for looking into it @mdboom!

For an experiment, I built cuda_core against main, and installed it along cuda_bindings built with this PR. (cuda_core cimports some things from cuda.bindings.driver). The cuda_core test suite passes. I /think/ that's sufficient proof that we don't have ABI issues, but lmk if that still may be hiding something we need to confirm works.

two notes here:

1. I think this should be the other way around: We build a project against older cuda.bindings (say 13.0.1), and run it with newer cuda.bindings (from this PR) installed in the venv. Xref: Building using versions of cuda-python with the new layout breaks runtime compatibility with older versions #274 (comment)
2. cuda-core is not a good testbed because it currently does not cimport cuda.bindings (by design; I see you asked a question on this in the other issue, I'll expand my answer there). Either RAPIDS/rmm or a simple Cython module like these should be sufficient to detect at least import issues

Great. I couldn't get rmm to work (I think I'm hitting this WSL bug), but I was able to:

• In a virtual environment with cuda-python installed from main, build all of the examples in cuda_bindings/tests/cython.
• Create a new virtual environment with cuda-python built from this PR, and import the exact same .so files created above without issue.