forked from PyO3/rust-numpy
-
Notifications
You must be signed in to change notification settings - Fork 0
Update pyo3 to 0.22.0 (without "py-clone") #1
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
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ment`. This trait can be trivially implemented for `Clone` types, but can also be implemented for types such as `PyObject` that can only be safely cloned while the GIL is held.
… for `Clone` types. The `impl_element_scalar` macro has been updated to invoke this new macro, and an explicit invocation was added for the types in the `datetime` module.
The impls were provided explicitly instead of using the `impl_py_clone` macro since making the macro flexible enough to handle the `const` param adds more complexity than it's worth.
This is the motivating impl for the changes, since `PyObject` can only be safely cloned while the GIL is held.
This is necessary in order to continue supporting object arrays after the update to PyO3 0.22.0 since `Py<T>` no longer implements `Clone` unless the 'py-clone' feature is enabled (which would be problematic for `numpy` to enable). This is a breaking change for 2 main reasons: 1. Custom `Element` impls will now need to provide a `PyClone` impl as well, since providing a blanket impl for all `T: Clone` is not possible. 2. A type parameter `T: Element` will no longer have an implicit `Clone` bound, so code relying on it will need to update the bounds to `T: Element + Clone`.
… token and call the `py_clone` method instead of `Clone`. Updated usages accordingly, which is trivial since the GIL is already held everywhere it is called.
…ad of `Clone` methods. This change is trivial since the GIL is already held everywhere that the clones occur, with the minor technicality that the GIL is not guaranteed to be held in the definition of the `PyArrayMethods` trait, but only in the actual impl block for `Bound<PyArray`. As such, several method implementations had to be moved to the impl block instead of being default implementations in the trait definition, but this is not a breaking change since the trait is `Sealed` and we control the only impl.
…yArray` impl block.
…ttribute. This is necessary since the `pyo3::pyobject_native_type_base` macro only implements `Debug` with the "gil-refs" feature since the 0.22.0 update. The `Debug` impls are no longer relevant anyway because `&PyArray`, `&PyArrayDescr`, etc. can only be obtained through the gil-refs API's.
… no longer necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds on PyO3#431 to migrate to PyO3 0.22.0 without needing to activate the problematic
py-clonefeature.Background
PyO3 recently released version 0.22.0 which includes many new features, but also includes the next step in deprecating the gil-refs API by putting it behind the opt-in
gil-refsfeature flag. @bschoenmaeckers's PR went long way towards updatingnumpyfor the migration by mirroring the gil-refs deprecation and feature-gating the methods that depend on it, but @juntyr raised the valid concerns about its unconditional activation of PyO3'spy-clonefeature flag.The problem with
py-cloneThe PyO3 0.22.0 release includes PyO3#4095's removal of the ability to clone
Py<T>while the GIL is not held, which was implemented by removing itsCloneimpl and requiring users to either convert toBound<T>before cloning or call theclone_refmethod instead. The opt-inpy-clonefeature flag was added that restores theCloneimplementation, but a panic is triggered if a clone is attempted while the GIL is not held. Enabling this feature may be convenient and fairly harmless in some use cases but can be hugely problematic in others, and we should not unconditionally impose the risk of panicking clones on allnumpyusers.Why not just add our own
py-clonefeature?This was suggested in PyO3#431, but has a major problem related to the
Elementimpl forPyObject(akaPy<PyAny>). The problem is thatCloneis a supertrait ofElement, soPyObjectlosing itsCloneimpl when thepy-clonefeature is disabled means itsElementimpl must be disabled as well. So if we made the feature opt-in as was done by PyO3, then by default we would lose all support for object arrays which would be a huge regression. Making the feature opt-out instead is also a bad option, so we need another way.Could we implement
ElementforBound<PyAny>instead?No. Unfortunately
Bound<T>does not (and can not) implementSend, which is another requirement for implementingElement. So retaining theElementimpl forPy<T>is our only option for supporting object arrays without major reworks.Eliminating our dependence on
py-cloneThe sad thing about this issue is that it really shouldn't be a problem in the context of
numpy-- all of our API's require the GIL anyways, so it should always be safe to clone thePyObjectsin our arrays. But that that doesn't help the fact that we still need a way tocloneelements (andto_ownedslices and array views), and we were relying on theElementtrait'sClonerequirement to do so. This PR solves the dilemma by replacing theClonesupertrait with a new traitPyClone, which can be safely implemented forPyObjectand all existingElementimpls.The new
PyClonetraitThe new
PyClonetrait is a weaker form of the standardClonetrait that represents types which can be cloned while the GIL is held. Philosophically (though not in practice), this includes allClonetypes that don't care about the GIL, as well as types likePy<T>that can only be cloned when it is held.The trait has the following (simplified) definition:
Any type that implements
Clonecan trivially implementPyCloneby ignoring thepyargument and simply delegating to itsclonemethod. Meanwhile,PyObjectcan implement this trait by using the providedPythontoken to callPy::clone_ref. The actualy trait also defines a couple of methods (not shown above) for cloning collections, which have default implementations that types can override if there is a more efficient way to do so then simply mapping thepy_clonemethod to each item.Adding a trait like this to PyO3 was discussed in PyO3#4133, but it is not clear if/when this would be implemented. If such a trait ends up getting added to PyO3 we can later transition to using it, but I don't think we should hold up
numpy's migration to 0.22.0 waiting for that to happen.Drawbacks
Ideally we would provide a blanket impl of
PyClonefor allT: Cloneand then a specific impl forPyObject, which would let us avoid the breaking change for anyone with customElementimpls that will now need to add aPyCloneimpl as well. Unfortunately, this is not allowed due to possible impl overlaps since the compiler asserts that PyO3 restoring theCloneimpl forPy<T>should not be a breaking change. IfPyClonetrait was instead defined in thepyo3crate then they could provide these impls, but it is unlikely that they would (as discussed in PyO3#4133) since it would prevent them defining necessary generic impls likeOption<T: PyClone>.So, this will inevitably need to be a breaking change for downstream crates defining custom
Elementimpls for their types. Luckily it extremely rare for crates to do this, and adding thePyCloneimpl is absolutely trivial in comparison to all the other changes needed to migrate away from the gil-refs API in the PyO3 0.21/0.22 updates anyways.Patching
numpyinternalsAs mentioned before, all of the
numpyAPI's already require access to aPythontoken to prove that the GIL is held, so most of the necessary updates simply require replacingitem.clone()calls withitem.py_clone(py). The only exceptions are the calls to<[T] as ToOwned>::to_ownedandArrayView::to_owned(both of which requireT: Clone), which now delegate toPyClone's provided methods discussed above. Delegating to these collection-specific methods is used instead of simply mapping thepy_clonemethod to avoid losing out on the optimizedto_ownedimplementations forClonetypes.Tasks
gil-refsAPIgil-refsfeature guarding all usages of PyO3's gil-refs API (completed by @bschoenmaeckers)py-clonefeature dependencyPyClonetrait to replaceCloneas a supertrait ofElementPyCloneimpls for all internalElementtypesnumpyinternals to usePyCloneinstead ofClonePyClonetraitElementdocs to reference the newly required supertraitPyClone-specific tests worth adding.