pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.34k stars 638 forks source link

PyO3: upgrade to v0.23.x #21657

Closed tdyas closed 23 hours ago

tdyas commented 3 days ago

Upgrade to v0.23.x of the pyo3 crate:

tdyas commented 2 days ago

GILProtected changes have been broken out to https://github.com/pantsbuild/pants/pull/21670.

tdyas commented 2 days ago

The commits each have a distinct change and can be reviewed individually.

huonw commented 1 day ago

(Just checking that the "[WIP]" in the PR title is no longer true? this is complete?)

tdyas commented 1 day ago

(Just checking that the "[WIP]" in the PR title is no longer true? this is complete?)

ah yeah, it is no longer WIP. Will edit.

tdyas commented 23 hours ago

Only question: with the newly fallible tuple constructor, do we know in what cases it might fail?

Just wondering if some more of them may count as fatal errors (i.e. use .expect(...) or panic!() more), or be normal/likely-to-occur and thus be worth wrapping in more context to make debugging easier?

I don't know when it is ever expected to fail. I want to say that "regular" Python and Rust std types are not expected to fail in practice, but I can't point to anywhere supporting that position. The migration guide just notes that "conversions can now return an error." And PyTuple::new is documented to panic if an iterator's ExactSizeIterator implementation is broken.

tdyas commented 23 hours ago

I'm going to land this; if need be, subsequent PRs can improve on how to handle PyTuple::new being able to fail.

huonw commented 12 hours ago

Thanks for the links, digging through into the source code suggests the fallibility is from IntoPyObject and the into_pyobject calls on each element (rather than something about the tuple itself): https://docs.rs/pyo3/0.23.1/src/pyo3/types/tuple.rs.html#103

So, I reckon what you've got here is perfectly fine 👍