Skip to content

[stdlib] Add PyCapsule_New and PyCapsule_GetPointer #4334

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[stdlib] Add PyCapsule_New and PyCapsule_GetPointer #4334

wants to merge 4 commits into from

Conversation

winding-lines
Copy link
Contributor

These are critical when interacting with PyCapsules created by other libraries in the system. The current use case is interacting with PyArrow.

@winding-lines winding-lines requested a review from a team as a code owner April 11, 2025 01:26
@winding-lines winding-lines changed the title Add PyCapsule_New and PyCapsule_GetPointer [stdlib] Add PyCapsule_New and PyCapsule_GetPointer Apr 11, 2025
@winding-lines winding-lines force-pushed the py-capsule-support branch 2 times, most recently from 721af64 to add0db0 Compare April 11, 2025 01:33
soraros
soraros reviewed Apr 11, 2025
View reviewed changes
mojo/stdlib/src/python/_cpython.mojo Outdated
Comment on lines 1955 to 1947
return self.lib.call["PyCapsule_New", PyObjectPtr](
pointer, name.unsafe_ptr(), destructor
)
Copy link
Contributor

@soraros soraros Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think you need to call self._inc_total_rc() here, since PyCapsule_New returns a new reference. Also, we should cast name to c_char, if only to document ABI compliance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, adding.

@soraros

This comment was marked as resolved.

Sign in to view
@winding-lines
Copy link
Contributor Author

Could you please put improve the docstring a bit more and add the signature of the Python C API function in a comment #4205 style?

Could you please put improve the docstring a bit more and add the signature of the Python C API function in a comment #4205 style?

Nice, I pushed a second commit. I am not sure if you prefer separate commits for ease of review or just one commit for cleaner history.

soraros

This comment was marked as resolved.

Sign in to view

@soraros

This comment was marked as outdated.

Sign in to view
@winding-lines
Copy link
Contributor Author

I think inline comments are important, as they help with eyeballing, especially when revisiting the code after a month.

Thanks for the edits, accepted them.

@JoeLoser JoeLoser requested a review from laszlokindrat April 16, 2025 16:28
laszlokindrat
laszlokindrat approved these changes Apr 23, 2025
View reviewed changes
Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Just a few nits and requests, but LGTM otherwise.

mojo/stdlib/test/python/test_python_cpython.mojo Outdated
Comment on lines 59 to 60
var capsule_pointer = Cpython_env[].PyCapsule_GetPointer(
capsule, "some_name"
)
assert_equal(capsule_impl.bitcast[NoneType](), capsule_pointer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a test for a PyCapsule_GetPointer with a mismatched name?

Suggested change
var capsule_pointer = Cpython_env[].PyCapsule_GetPointer(
capsule, "some_name"
)
assert_equal(capsule_impl.bitcast[NoneType](), capsule_pointer)
var capsule_pointer = Cpython_env[].PyCapsule_GetPointer(
capsule, "some_name"
)
assert_equal(capsule_impl.bitcast[NoneType](), capsule_pointer)

@winding-lines
Add PyCapsule_New and PyCapsule_GetPointer
e42979d 
These are critical when interacting with PyCapsules created by other
libraries in the system. The current use case is interacting with
PyArrow.

Signed-off-by: Marius Seritan <[email protected]>
winding-lines and others added 3 commits April 23, 2025 17:20
@winding-lines @laszlokindrat
Update mojo/stdlib/test/python/test_python_cpython.mojo
b8a7933 
Co-authored-by: Laszlo Kindrat <[email protected]>
Signed-off-by: Marius Seritan <[email protected]>
@winding-lines @laszlokindrat
Update mojo/stdlib/test/python/test_python_cpython.mojo
45d677e 
Co-authored-by: Laszlo Kindrat <[email protected]>
Signed-off-by: Marius Seritan <[email protected]>
@winding-lines
Address feedback
affaddd 
Signed-off-by: Marius Seritan <[email protected]>
@winding-lines
Copy link
Contributor Author

@laszlokindrat many thanks for the review, I addressed the feedback. I am not sure if there is anything else for me to do, I will wait for the PR to be merged at some point. Let me know if I am missing something.

@laszlokindrat
Copy link
Contributor

!sync

laszlokindrat
laszlokindrat reviewed Apr 24, 2025
View reviewed changes
@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Apr 24, 2025
@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the main branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label Apr 24, 2025
@modularbot
Copy link
Collaborator

Landed in 86a9310! Thank you for your contribution 🎉

@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Apr 26, 2025
@modularbot modularbot closed this Apr 26, 2025
modularbot added a commit that referenced this pull request May 6, 2025
@modularbot @winding-lines
[External] [stdlib] Add PyCapsule_New and PyCapsule_GetPointer (#60343)
53d3e9b 
[External] [stdlib] Add PyCapsule_New and PyCapsule_GetPointer

These are critical when interacting with PyCapsules created by other
libraries in the system. The current use case is interacting with
PyArrow.

<!--
Thanks for submitting a pull request,
your contribution is really appreciated!

If possible, add a link to the issue you are
trying to solve in the pull request description.

If your pull request is big (> 100 lines), consider splitting it
into multiple pull requests as it may accelerate the review process.
-->

ORIGINAL_AUTHOR=Marius Seritan
<[email protected]>
PUBLIC_PR_LINK=#4334

Co-authored-by: Marius Seritan <[email protected]>
Closes #4334
MODULAR_ORIG_COMMIT_REV_ID: 4a1e4aa8c12b799de2eb010e75fda4f4f64b76c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soraros soraros soraros left review comments

@laszlokindrat laszlokindrat laszlokindrat approved these changes

Assignees

@laszlokindrat laszlokindrat

Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@winding-lines @soraros @laszlokindrat @modularbot @ConnorGray