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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[stdlib] Add PyCapsule_New and PyCapsule_GetPointer #4334

wants to merge 4 commits into from
+70 −3

Conversation

winding-lines
Copy link

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
Author

Choose a reason for hiding this comment

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

Oops, adding.

@soraros
Copy link
Contributor

soraros commented Apr 11, 2025

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?

@winding-lines
Copy link
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
Copy link
Contributor

soraros commented Apr 11, 2025 •
edited
Loading

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.

I think they get squashed when imported into their internal repo anyway, so it shouldn't matter much.

@winding-lines
Copy link
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
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.

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
None yet
Projects
None yet
Milestone
No milestone
Development

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