-
Notifications
You must be signed in to change notification settings - Fork 784
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
remove internal APIs from pyo3-ffi #4201
Conversation
Looks like CI is failing due to a warning from an unused macro. |
Sure, this PR is still WIP. I will work on it more soon. |
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 very much for working on this! I think there's a little bit more we can do as part of this one:
- I see a use of
_Py_c_neg
around line 154 which we can update - Also a use of
_Py_c_abs
around line 288 to update - Let's add
newsfragments/4201.changed.md
to note that the implementation has changed here even if the API remained the same, as there will be a performance impact.
I also wonder if we should add a TODO here or in a follow-up issue to discuss whether in the long term these operations should be removed? Given that CPython doesn't guarantee these methods won't fail I somewhat wonder if we should peel these away and encourage users to fall back to the PyAnyMethods
functions. Panics are never nice.
For a first step though, it's very good for us to get off the CPython private APIs!
@davidhewitt happy to do it either here or open a new PR for the suggestion |
Given this PR is still relatively self-contained, I'd be happy for the additional changes to be pushed here 👍 |
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.
Looking good, thanks, just a final thought (which might create some more ideas... 👀)
The idea to add to PyAnyMethods
- I'm fine for it to be either part of this PR, a separate PR which we merge first, or a follow-up.
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.
Super, this looks great, thanks!
Just a couple of final suggestions to tidy up the PyAnyMethods
additions, and then let's merge 🚀
newsfragments/4201.changed.md
Outdated
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.
Let's also add 4102.added.md
please with something like "Added PyAnyMethods::{abs, pos, neg}
."
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.
Sure!
src/types/any.rs
Outdated
/// Computes the positive of self. | ||
fn pos(&self) -> PyResult<Bound<'py, PyAny>>; | ||
|
||
/// Computes the absolute of self or `|self|`. |
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.
Here I think the Python equivalent is abs(self)
.
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
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, this looks good to me! Really pleased to get this dependency on the private API removed 👍
@Cheukting Could you have a look into the build error on GraalPy from our CI? If you'd like, we can also label this PR so the cull CI suite is run on every push so you do not have to wait for the merge queue to validate your changes? |
@adamreichold it is due to an unused import it seems: https://github.com/PyO3/pyo3/actions/runs/9336538447/job/25697227447#step:10:132 I will have a look into it, thanks! |
@adamreichold do you mind running the check on the merge queue again to see if my fix works? |
Using methods from PyAny to replace calling _Py internal APIS from pyo3-ffi.
see #3762