Prepare for CTK 13.4 - #2423
Conversation
|
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. |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
rwgk
left a comment
There was a problem hiding this comment.
Based on a codex gpt-5.6-sol analysis:
I took a closer look at the new raw-storage checks because I was unsure what behavior they were intended to cover.
For CUipcMemHandle_st(), neither CTK nor the driver performs the zero-initialization: the test does not call a CUDA API. The wrapper embeds a C struct in the Cython extension object, Cython/CPython zero-initializes that object storage before __cinit__, and __cinit__ merely points the wrapper at the embedded struct. There is no explicit clearing operation in cuda_bindings.
This makes the replacement for test_char_range somewhat questionable to me:
- The old test exercised the exposed
reservedsetter and itscharconversion behavior. Once that property is intentionally omitted, the replacement does not preserve the same coverage. - The new test primarily verifies Cython's object-allocation behavior by reaching through
getPtr()withctypes. - A default-constructed all-zero
CUipcMemHandle_stis not itself a valid CUDA IPC handle, so the user-visible value of this invariant appears limited.
The graph tests are more valuable: their primary purpose is to ensure that returned edge-data wrappers outlive the temporary buffers used by the binding. However, I think their public-field checks and heap stirring already exercise that regression. Reading the hidden reserved bytes through ctypes is ancillary and hard-codes the eight-byte layout and the offset at which the reserved storage begins.
My recommendation would therefore be to delete test_char_range, retain the four graph lifetime tests, and remove only their raw reserved-byte assertions. If zero-initialized wrapper storage is intended to become an explicit cuda_bindings contract, I would prefer to make the initialization explicit in generated code and cover that contract with one focused test.
I do not consider this blocking for the PR and am leaving the final choice to you.
I think this is a good idea. I had replaced the existing tests with like behavior on the assumption that otherwise would be seen as a reduction in test coverage. I absolutely agree that it's better to not enforce a contract we don't want to make. I'll make these changes and then merge. |
This updates the generated files with a number of bugfixes that were found while making CTK 13.4 work. We should merge this first so we can plainly see 13.4-only changes when we are really to work on that.
See MR 495 for the source of these changes.
This includes the addition of 4 new NVML APIs from CTK 13.3 that had never been implemented before.