-
Notifications
You must be signed in to change notification settings - Fork 198
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
Only use functions in the limited API #1545
Only use functions in the limited API #1545
Conversation
In addition to base rmm testing, compilation under the limited API is tested in rapidsai/devcontainers#278. Note that CI failed when using rmm 24.06, then successfully compiles using this branch. |
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.
Thanks Vyas! 🙏
The original idea of PyBytes_AS_STRING
was to indirectly do struct
member access (skipping type checking that PyBytes_AsString
does), which should be fast
If we want to drop PyBytes_AS_STRING
for limited API support (which is reasonable), maybe we should drop the function call altogether and let Cython do this for us. This would improve readability and should do the same thing under-the-hood
Co-authored-by: jakirkham <[email protected]>
Right, but the more relevant question I think is whether that performance difference is even visible or if it's lost in all the other noise of rmm (let alone the noise of other RAPIDS packages that use rmm). I ran some quick tests doing something like a few thousand calls to |
Agreed. That was just historical context. Am ok dropping it. For readability alone it seems worth it |
/merge |
Thanks Vyas! 🙏 |
Description
This PR removes usage of the only method in rmm's Cython that is not part of the Python limited API. Contributes to rapidsai/build-planning#42
Checklist