add FromPyObjectBound trait for extracting &str without GIL Refs

[davidhewitt]

This is an attempt to solve the problem that FromPyObject for &str needs GIL Refs for versions under 3.10, and also can't be expressed until we add a second lifetime to FromPyObject because the data needs to be borrowed from the input, not from 'py.

I do this by creating a new trait FromPyObjectBound which is expected to be the future form of FromPyObject. I think for now we recommend users not to implement it unless they have the same borrow problem.

For backwards compatibility reasons I move as little code as possible onto this new trait. Just .extract() methods and also argument extraction code use this new trait internally.

On the implementation side, this trait gets a blanket for anything implementing FromPyObject.

Finally, without the gil-refs feature we remove the existing implementation of FromPyObject for &str and instead implement this new trait. This works except for abi3 on old Python versions, where we have to settle for PyFunctionArgument only and allow users of .extract() to break.

This does implement some very targeted breakage for specific considerations, but I wonder if this is necessary as the correct way to users off the GIL Ref API in a case where otherwise it's near impossible for them to realise this uses it.

I can't see a way to do this any gentler.

[davidhewitt]

This does need changelog as it implements FromPyObject for Cow<str>.

[davidhewitt]

As a final thought, I would have said for performance reasons alone this breakage would be unnecessary, but for #3668 safety reasons this seems worth it to avoid users unsuspectingly still having problems.

cc @alex as I think the proposed change to FromPyObject for &str will affect cryptography.

[alex]

Yeah this will definitely impact us. Thanks for flagging.

[davidhewitt]

@alex as a user do you think you'd rather have a smooth experience upgrading and know that there are some GIL Ref uses internally, or have a little pain and get cryptography completely migrated?

(The pain will only come on deactivating the gil-refs feature, and I should probably make the migration guide entry better. I ran out of time this morning...)

[alex]

From my perspective, our migration process is going to be:

1. Get upgraded with gil-refs enable
2. Burn down all uses
3. Disable gil-refs

I don't think it's super useful to have "well you disabled gil-refs, but actually they're secretly still on in the background".

And yes, the fact that extract::<&str>() is not going to work is annoying, and will cause some pain. But we already have the fallback option of enabling gil-refs :-)

[davidhewitt]

Great, that is how I'm thinking about the migration too. I will aim to get this PR cleaned up with a better migration guide entry shortly.

[codspeed-hq]

CodSpeed Performance Report

Merging #3928 will improve performances by 50.86%

_{Comparing davidhewitt:frompyobject (4dad333) with main (fbd5311)}

Summary

⚡ 2 improvements
✅ 65 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	davidhewitt:frompyobject	Change
⚡	extract_str_extract_fail	2.8 µs	2.3 µs	+22.21%
⚡	extract_str_extract_success	978.9 ns	648.9 ns	+50.86%

[Icxolu]

Generally this looks like a good approach for the migration period. I was wondering whether we should even allow downstream implementations of FromPyObjectBound. Since this is pretty much the next form of FromPyObject (except for the name), maybe it's fine to leave this escape hatch open. I guess only the names would need updating once we remove the current FromPyObject and replace it with this one.

[Icxolu]

Any particular reason to not switch to new_bound_with instead now? (Same below)

[davidhewitt]

None other than me being a bit sloppy! To make things more manageable at the beginning of the migration I put #[allow(deprecated)] on quite a few of the test modules, which I think we will need to clean up on the way to 0.22.

[Icxolu]

We could also already call this from_py_object, but maybe it's better to have a clear distinction for now...

[davidhewitt]

I think the one question which made me hesitate on encouraging users to use this trait too much (and also make this name clunky) is that I think to resolve the TODO in instance.rs:

        // TODO it might be possible to relax this bound in future, to allow
        // e.g. `.extract::<&str>(py)` where `py` is short-lived.
        'py: 'a,

we would need to make this trait accept ob: Borrowed<'a, 'py, PyAny> as the input, as that's basically &Bound with additional flexibility on the relationship between 'a and 'py.

It felt to me that this is a hurdle we can cross down the line, and working with Bound for now keeps it simple...

[Icxolu]

Ah, using Borrowed there would certainly be an option. I agree that this is a question best answered once we actually removed the gil-refs and do the final migration. We could also seal this trait for now (but maybe that's a bit drastic). Removing a seal in a patch release, if it turns out to be necessary, would also be an option.

[davidhewitt]

Let's seal this, and leave a note saying that "if you have a need to use this trait, please let us know and help us work out the design".

[davidhewitt]

Ok, I pushed an additional commit to seal the trait.

[davidhewitt]

Coverage aside (which I'm now resigned to being a bit lax on while the gil-refs API leads to horrendous duplication), I think this is now good to review.

I'm aiming to draft (probably tomorrow evening) a few notes for the documentation. After that, and with this merged, I think it's time we shipped a beta release!

[davidhewitt]

Just thinking further on this, I think the alternative would be to make one small breaking change to FromPyObject immediately: the lifetime.

If we changed the definition of FromPyObject to be

pub trait FromPyObject<'a, 'py>: Sized {
    fn extract(ob: &'py PyAny) -> PyResult<Self> {
        Self::extract_bound(&ob.as_borrowed())
    }

    fn extract_bound(ob: &'a Bound<'py, PyAny>) -> PyResult<Self> {
        Self::extract(ob.clone().into_gil_ref())
    }

    #[cfg(feature = "experimental-inspect")]
    fn type_input() -> TypeInfo {
        TypeInfo::Any
    }
}

that is, we add the lifetime 'a to FromPyObject now, but otherwise changed nothing about it, that would remove the need for FromPyObjectBound.

We could explain in the migration guide exactly how users update their code. I think the two main cases would be:

• in implementations of FromPyObject, which could be solved immediately by using '_ for the lifetime:

  // before
  impl<'py> FromPyObject<'py> for T {
  
  // after
  impl<'py> FromPyObject<'_, 'py> for T {

• in trait bounds, where users could use an HRTB for now:

  // before
  fn foo<'py, T>(obj: &'py PyAny) -> T
      where T: FromPyObject<'py>
  
  // after
  fn foo<'py, T>(obj: &'py PyAny) -> T
      where T: for<'a> FromPyObject<'a, 'py>

I think this would be acceptable breakage in my eyes if it leaves PyO3 in a more consistent state. I actually suspect that many users don't name the FromPyObject trait directly much at all. In pydantic-core there are only 14 lines which would need to be changed for this out of 23.5k.

---

Maybe the one unresolved problem would be how the #[derive(FromPyObject)] proc macro allows users to set the additional 'a lifetime in the generated code. I think it would be quite reasonable to punt on that initially, and continue to only support the one 'py lifetime in 0.21. We could fix that either as a patch or part of 0.22.

Ungh, I really want to get that beta release out, but also I think the decision about what to do here is worth doing correctly...

😂

[davidhewitt]

Ok, so I tried the above but it doesn't work because that proposed definition of FromPyObject<'a, 'py> with those default methods falls apart. The compiler starts needing 'py: 'a for the default implementation of fn extract.

So I think that we cannot achieve that proposal in 0.21 without more breakage. I think this confirms that such a change will need to wait to 0.23 when we remove fn extract GIL Ref form from the trait entirely.

Glad I explored that, as it makes me confident that proceeding with this PR is the right approach.

[Icxolu]

I believe I have also explored this in the past (maybe during the initial FromPyObject migration), but also ran into issues (not sure anymore whether it was that specific one). Sticking with this approach for the migration period sounds good to me.

[Icxolu]

LGTM