Skip to content
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

implement PySequenceMethods, try 2 #3572

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

davidhewitt
Copy link
Member

This is a reboot of #3535 with a second commit to attempt to settle the suggestion in #3535 (comment)

I played around with a few different designs of how to abstract out some of the repeated logic from the FFI calls, such as the second commit I pushed to that PR.

Ultimately, I settled on adding a pair of crate-private extension traits, impl FfiPtrExt *mut ffi::PyObject and impl PyResultExt for PyResult<Py2<'_, PyAny>>. (These traits could be made public in the future if we wanted, but that's not a question I want to cover right now.)

It's best to see how these traits play out for example with the implementation of repr():

    fn repr(&self) -> PyResult<Py2<'py, PyString>> {
        unsafe {
            ffi::PyObject_Repr(self.as_ptr())
                .assume_owned_or_ffi_error(self.py())  // *mut ffi::PyObject -> PyResult<Py2<'py, PyAny>>
                .downcast_into_unchecked()  // PyResult<Py2<'py, PyAny>> -> PyResult<Py2<'py, PyString>>
        }
    }

Overall, I like how this reads top-to-bottom, unlike the previous implementations where we often had py.from_owned_ptr() wrapping the ffi call. I also wanted to keep the downcast explicit, especially unchecked downcasts, because those are assumptions on the underlying ffi call and it makes it easier to audit the implementation by keeping it all explicit.

Of the iterations I tried, this also was a reasonable fit for code size / verbosity without being either too short and magical or too long and complex.

I'm very happy to bikeshed names, these seemed to work for me though I'm sure there are better names out there.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Nov 13, 2023
@davidhewitt
Copy link
Member Author

davidhewitt commented Nov 15, 2023

@adamreichold I'm sufficiently happy with this that I'm planning to implement all of the Py*Methods traits for our types over the coming weeks using this style.

FYI all, @pydantic have allocated me a two week block to get far enough along with an implementation that we can use it in pydantic-core, so I hope to churn out a few of these "implement PyXMethods" PRs over the coming days. (We'll build pydantic-core from my branch in the short term while things get reviewed on the way to 0.21.)

Copy link

codspeed-hq bot commented Nov 15, 2023

CodSpeed Performance Report

Merging #3572 will not alter performance

Comparing davidhewitt:sequence2-try2 (82ac801) with main (fc82c9f)

Summary

✅ 78 untouched benchmarks

@davidhewitt
Copy link
Member Author

Fellow maintainers, I'm relatively keen to merge this because the rest of the PyXMethods traits can then be implemented immediately in this style without needing to have later churn.

Any thoughts or feelings on the style introduced by 907bd34 before I do so?

As it's an internal-only refactor I'm not too worried about the exact design here, we can change it again any time in future. Still, it would be nice to know I'm merging something we like as a group rather than something which is optimised purely for my own eyes 🙈

@adamreichold
Copy link
Member

I understand that lack of feedback is highly frustrating, but for me I just did not yet find the time to look into this even though it is on top of my "large stuff" to-do list. (But I only find short breaks, i.e. only items from my "small stuff" to-do list get worked on.)

As it's an internal-only refactor I'm not too worried about the exact design here, we can change it again any time in future. Still, it would be nice to know I'm merging something we like as a group rather than something which is optimised purely for my own eyes 🙈

But personally, I don't think the above approach will work. For exactly the same lack of time that prevents substantial review of these changes, nobody will realistically be able to substantially alter the design after it was merged and new changes are piling up waiting to be reviewed.

So I think we should be honest with ourselves here, which includes acknowledging the large difference in time available for PyO3 for different maintainers ATM. From my point of view, this does not preclude merging this and pushing on because I consider paying for influence on the project direction by putting in the work somewhat reasonable. But if we really want to this to be based on some degree of consensus, it should be blocked from merging until getting reviewed by at least one other maintainer.

@davidhewitt
Copy link
Member Author

It's a difficult balance. I also frequently have periods of time where I am not able to keep up with PyO3's development & related discussions, so I understand the to-do list pain all too well. I'm keen to make the most of this phase where I'm finding a little bit more time than I have in the past!

As maintainers we all have invested plenty of personal time and effort into this project; I think it's important to respect that and allow folks time to (find time to) voice their opinions. Also, reviews definitely improve the quality of my PRs, and I am thankful for every review I receive 😄

Sure, waiting some time for review does slow development speed a little. That's fine; it's an important part of the process. I can keep working by stacking up revisions on branches and then unstack them into PRs in an order which makes sense to review and merge. We also have many different threads of development so often I just switch gears to a different task.

My recent pattern has been to give PRs which I want to make progress on somewhere around a few days to a week before I ping. Depending on the PR, that ping goes one of two ways:

  • For PRs which are more user-facing, or where there's space in the design, the ping is just a ping to help guide where I'm hoping for input. Sometimes I try to split the PR so that the bits I feel need more careful review are isolated into smaller PRs.
  • If the PR is relatively straightforward (or internal code), I've been adding justification why I'm ok to merge without waiting further for review (and accept the responsibility for negative fallout from skipping review is mine to bear). My hope is this helps reduce review burden to leave just those PRs where putting heads together will add the most payoff. What I don't want is for folks to feel cut out of the loop because I merged without waiting longer.

I'm open to hearing what feels ok with fellow maintainers, or if I should be waiting longer to ping, or any other thoughts how I should adjust what I've been doing.


As for this PR, I can easily work off branches based on top of it, so given that you have a strong intention to review this, I can wait.

nobody will realistically be able to substantially alter the design after it was merged and new changes are piling up waiting to be reviewed

I do want to say that if we did merge this PR and then we later had discussion which concluded that this implementation pattern needed replacing, I'd feel responsible to put in the effort and time to resolve it. I think it's important to defer to our group opinion; this project is not mine to own.

IMO, for internal code, merging to main doesn't need to mark the design immutable. As long as we're comfortable to have functionality built on top of the code in question (and potentially release it in a production environment), then that's good enough.

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Some suggestions on the names of things, otherwise looks good to me.

Having to duplicate all method doc comments is unfortunate, but should be temporary if I understand things correctly? Also Git really tried hard to make the diff less readable. :(

use ffi_ptr_ext_sealed::Sealed;

pub(crate) trait FfiPtrExt: Sealed {
unsafe fn assume_owned_or_ffi_error(self, py: Python<'_>) -> PyResult<Py2<'_, PyAny>>;
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer the naming to match what it forwards to, i.e. assume_owned_or_err and assume_owned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, I don't really love the owner_or_err names but I guess at this point they are more or less entrenched, let's keep things uniform for now and if I want to pull at that thread another day, I'll do so 😂

src/types/any.rs Outdated
@@ -12,6 +13,8 @@ use std::cell::UnsafeCell;
use std::cmp::Ordering;
use std::os::raw::c_int;

use crate::py_result_ext::PyResultExt;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go into the crate block above with FfiPtrExt.

@davidhewitt
Copy link
Member Author

Thanks for the review here, really appreciate it 🙏

I'll fixup this now, merge it, and then rebase #3606. I think my follow-up action will then be to split off from that a few follow-up PRs with similar mechanical additions like PyMappingMethods and so on.

@davidhewitt davidhewitt force-pushed the sequence2-try2 branch 2 times, most recently from 64d1025 to 090d7e1 Compare December 14, 2023 10:51
@davidhewitt davidhewitt added this pull request to the merge queue Dec 14, 2023
Merged via the queue into PyO3:main with commit 79a54cf Dec 14, 2023
@davidhewitt davidhewitt deleted the sequence2-try2 branch December 14, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants