implement PyTupleMethods

[davidhewitt]

Split off from #3606

There are a few interesting pieces going on here, because it is safe to borrow items from a tuple due to its immutability:

• Iterating tuples with the gil-ref API yields borrowed objects. This however isn't possible when calling .into_iter() on a Py2<PyTuple> because the reference is then owned by the iterator, and we don't have lending iterators in Rust yet.
  • For this reason, I introduced PyTupleIterator2 as yielding owned objects.
  • Users can opt-in to borrowed iteration with an .iter_borrowed() method, which creates an iterator PyTupleIterBorrowed which yields Py2Borrowed items. The gil-ref iterator wraps this borrowing iterator.
• Similarly the gil-ref API get_item borrows objects. To keep types predictable on the migration from &PyAny -> Py2<'py, PyAny> the new get_item returns owned objects, and I have added get_borrowed_item which returns borrowed objects and is what the gil-ref API wraps.

This way the more efficient "borrowed" methods are still available, but users have to opt-into them. I'd argue this creates a simpler migration pathway while also leaving the more efficient operations available for us to use.

Finally, it turns out that to implement this it starts getting useful if Py2Borrowed implements Copy (which seems reasonable as it's basically a pointer), so I've pushed that as a precursor commit.

[codspeed-hq]

CodSpeed Performance Report

Merging #3681 will improve performances by 24.39%

_{Comparing davidhewitt:tuple2-try2 (4cf58c8) with main (54390bc)}

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	davidhewitt:tuple2-try2	Change
⚡	sequence_from_tuple	425 ns	341.7 ns	+24.39%

[davidhewitt]

@adamreichold I've rebased this PR on all the changes we've merged recently.

I also pushed two additional commits to make this PR go further than the other PyXMethods PRs.

1. I introduce the PyTuple::new_bound and PyTuple::empty_bound methods
2. I introduce a no-pool feature which is similar to Hide GILPool behind a feature to test ability to fully avoid it #3685

The interesting idea I had is that with the no-pool feature enabled, we can also replace entirely the old names PyTuple::new and PyTuple::empty with their new forms with return Bound<'_, PyTuple>.

There's two nice upsides to this:

• Users who are prepared to put in the work to completely drop the pool can also then immediately opt-in to the future API names without waiting for us to do a great remove-and-replace release. This makes that future release less breaking.
• It makes it easy for us to re-use the existing unit tests for both variants of the API depending on whether the no-pool feature is active.

[adamreichold]

The interesting idea I had is that with the no-pool feature enabled, we can also replace entirely the old names PyTuple::new and PyTuple::empty with their new forms with return Bound<'_, PyTuple>.

I have not yet looked at the code in question, but from the description I have to admit that I am not enthusiastic:

• Feature should be additive, i.e. add API and not remove them. Consider a third party crate still depending on the pool API but the top-level removing them by enabling a no-pool feature. This makes for a bad developer experience and will likely lead to unnecessary bug reports.
• I also dislike a feature changing methods instead of adding them, i.e. dependencies which do want the pool will access the same functionality under different names if they also interact with the new API. The PyO3 code base already feels a bit like a Rust version of #ifdef hell due to the need to support different Python versions, variants and API levels. I would prefer if we did not add to that if we can avoid it.
• I think the reuse is in the unit tests is somewhat accidental, i.e. it works only if the tests happen to never break types when ::new or ::empty change their type which give us some coverage for free, but it also raises the bar for understanding and extending these tests.

To me, it feels like we are continuously oscillating between "this will already be a hefty migration, let's avoid any unnecessary breakage" and "that pool-free future is glorious and I want to take everybody there with us, now". Personally, I am strongly in the first camp, I'd like us to use the next release to introduce the new API and their significant benefits to downstream projects but leave it as a completely optional thing that projects can look into at their own pace. Even deprecating the non-pool API is a pretty strong approach IMHO, but at least that can be easily silenced using #![allow(deprecated)].

[davidhewitt]

Hmm, yes I take your point. I think the thing that pains me is that it feels like we will be publishing 0.21 (and probably 0.22) with an API story that is very much "this is in migration, and we'll be asking you to join us in finishing off migration in a future release". And maybe that's just the inevitable reality of it.

I agree that this particular proposal of a no-pool feature is a bit of an abuse of how features work, and probably that's a good enough reason that we shouldn't do this.

I'll drop that last commit and let's continue to focus on getting the API feature complete first.

[davidhewitt]

Looking at how these deprecations are affecting even just our own code, I'm beginning to think maybe we should go even slower and not even deprecate stuff? #3703 (comment)

As keen as I am to get everyone onto a safer and more performant API, I think patience and good documentation of how and why the changes are occurring might be a better route than trying to encourage a rapid migration.

[adamreichold]

Looking at how these deprecations are affecting even just our own code, I'm beginning to think maybe we should go even slower and not even deprecate stuff? #3703 (comment)

As keen as I am to get everyone onto a safer and more performant API, I think patience and good documentation of how and why the changes are occurring might be a better route than trying to encourage a rapid migration.

Yes, I think shipped without any deprecations of the GIL ref API would be a better experience for our downstream projects. The main reason, I would like to add them is that this is usually part of our removal story, i.e. deprecate in version N and remove in version N+2. So if do not do this, we need to wait until N+3 for removal? Our maybe we use something like the pinned issue to tell people that we did not deprecate but still plan to remove in N+2 after deprecating in N+1?

(If we add a pool feature, downstream projects which want to make sure to migrate could still make sure they fully migrate. But of course, a warning is nicer than an error for the last three usages of GIL refs out of 243 original ones. Maybe a feature that will enable deprecations of the GIL ref API? But that feels like an abuse of features as well?)

[davidhewitt]

Going off the pool feature idea, maybe this: in 0.21 the feature just turns off the deprecation warnings, i.e. makes these APIs fully available.

In 0.22 we go further and gate the APIs behind the pool feature, and even with pool feature these APIs are deprecated.

And then in 0.23 (or maybe 0.24 if we feel conservative at the time) we remove the APIs and the feature entirely?

I think that should both create an additive feature and also hopefully a realistic vision for how we can ship 0.21?