FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use new faster auto lowpp class implementation by mdboom · Pull Request #1244 · NVIDIA/cuda-python · GitHub

/ cuda-python Public

Use new faster auto lowpp class implementation - #1244

Merged
mdboom merged 16 commits into
NVIDIA:mainfrom
mdboom:fast-auto-lowpp-class2
Dec 3, 2025
ConversationCommits (16)ChecksFiles changed
Merged

Use new faster auto lowpp class implementation#1244
mdboom merged 16 commits into
NVIDIA:mainfrom
mdboom:fast-auto-lowpp-class2

Conversation

mdboom commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

This is still somewhat a WIP, but I would like to get more CI.

copy-pr-bot Bot commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

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 commented Nov 17, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

This comment has been minimized.

Sign in to view
mdboom force-pushed the fast-auto-lowpp-class2 branch from 468f640 to 06930f5 Compare November 17, 2025 16:55
mdboom force-pushed the fast-auto-lowpp-class2 branch from 06930f5 to f01c309 Compare November 17, 2025 19:14

mdboom commented Nov 17, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

leofang reviewed Nov 17, 2025
View reviewed changes
Comment thread cuda_bindings/cuda/bindings/cycufile.pxd Outdated
leofang added enhancement Any code-related improvements P0 High priority - Must do! cuda.bindings Everything related to the cuda.bindings module labels Nov 18, 2025
leofang added this to the cuda-python 13-next, 12-next milestone Nov 18, 2025

mdboom commented Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom commented Nov 24, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom commented Nov 24, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

leofang reviewed Nov 24, 2025
View reviewed changes

leofang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Left a few quick questions, but otherwise LGTM. I think this PR can be un-drafted now? @mdboom do you plan to add more changes? Maybe you want to piggyback the dtype fix here?

Comment thread cuda_bindings/cuda/bindings/cufile.pyx Outdated
Comment thread cuda_bindings/cuda/bindings/cufile.pyx
leofang mentioned this pull request Nov 27, 2025
Closed

mdboom commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

leofang mentioned this pull request Dec 1, 2025
Merged

mdboom commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

I have confirmed with the tool in #1067 that there is no change in the ABI.

Unfortunately, the changes in this PR are /not/ tested in CI, given this on all of the tests that use these types. We will need to perform some manual testing.

@pytest.mark.skipif(not isSupportedFilesystem(), reason="cuFile handle_register requires ext4 or xfs filesystem")

mdboom marked this pull request as ready for review December 2, 2025 20:03

copy-pr-bot Bot commented Dec 2, 2025

Copy link
Copy Markdown
Contributor

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 force-pushed the fast-auto-lowpp-class2 branch from 5507171 to 104a5bb Compare December 2, 2025 21:52

mdboom commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

rwgk reviewed Dec 2, 2025
View reviewed changes
Comment thread cuda_bindings/docs/source/release/13.X.Y-notes.rst Outdated
leofang reviewed Dec 2, 2025
View reviewed changes
Comment thread cuda_bindings/cuda/bindings/cufile.pyx Outdated
return self._data.read_size_kb_hist
cdef view.array arr = view.array(shape=(32,), itemsize=sizeof(uint64_t), format="Q", mode="c", allocate_buffer=False)
arr.data = <char *>(&(self._ptr[0].read_size_kb_hist))
return arr

leofang Dec 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Q: Can we wrap it as a numpy array here to avoid breaking?

Comment thread cuda_bindings/cuda/bindings/cufile.pyx Outdated
return self._data.write_size_kb_hist
cdef view.array arr = view.array(shape=(32,), itemsize=sizeof(uint64_t), format="Q", mode="c", allocate_buffer=False)
arr.data = <char *>(&(self._ptr[0].write_size_kb_hist))
return arr

leofang Dec 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ditto

mdboom marked this pull request as draft December 3, 2025 12:42

mdboom commented Dec 3, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom commented Dec 3, 2025

Copy link
Copy Markdown
Contributor Author

I have reverted the backward compatibility by returning numpy arrays, rather than Cython arrays, from members that have numeric array types.

I tested this again on a machine that doesn't skip the cufile tests and all is passing there.

There is still one breaking change here, noted in the release notes, that IMHO is just a real bug -- PerGpuStats was declared as a AUTO_LOWPP_CLASS rather than an AUTO_LOWPP_ARRAY, but it is always used as an array. You can see in the test code the weird backflips that were required to access it as an array that go away if you just declare it as such. Unfortunately, this is required here because I can't make PerGpuStats[0] return a Numpy array as before under the new implementation, but that never really made sense anyway.

mdboom commented Dec 3, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test

mdboom marked this pull request as ready for review December 3, 2025 14:47
mdboom merged commit f04df74 into NVIDIA:main Dec 3, 2025
117 of 119 checks passed

github-actions Bot commented Dec 3, 2025

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

mdboom deleted the fast-auto-lowpp-class2 branch December 9, 2025 16:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rwgk rwgk left review comments
leofang leofang approved these changes
kkraus14 kkraus14 approved these changes

Assignees

mdboom

Labels

cuda.bindings Everything related to the cuda.bindings module enhancement Any code-related improvements P0 High priority - Must do!

Projects

None yet

Milestone

cuda-python 13.1.0 & 12.9.5

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL