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

PyO3: upgrade to v0.23.x #21657

Merged
merged 22 commits into from
Nov 21, 2024
Merged

PyO3: upgrade to v0.23.x #21657

merged 22 commits into from
Nov 21, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Nov 18, 2024

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

  • Functions in PyO3 with _bound suffixes existed in PyO3 only for easing migration to the Bound API. They are deprecated now and new methods without the _bound suffixes have been re-introduced in PyO3. This PR renames call sites accordingly and updates code to also reflect that some of the new APIs (e.g., PyTuple::new) are now fallible.

  • The IntoPy and ToPyObject traits are deprecated in favor of the new IntoPyObject trait (which is fallible). To ease migration, this PR only disables deprecation warnings as errors in the affected files. (Unfortunately, IntoPyObject was not introduced in the current v0.22.x version and so we cannot migrate ahead of the upgrade (unlike what was done in use GILProtected to synchronize access to various pyclass types #21670 for the new pyclass Sync requirement).) Migration will take place in follow-on PRs.

    • This PR does add an implementation of IntoPyObject for &Value to support some existing call sites.

@tdyas tdyas added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes labels Nov 18, 2024
@tdyas
Copy link
Contributor Author

tdyas commented Nov 19, 2024

GILProtected changes have been broken out to #21670.

@tdyas tdyas mentioned this pull request Nov 19, 2024
4 tasks
@tdyas tdyas marked this pull request as ready for review November 20, 2024 11:26
@tdyas tdyas requested a review from benjyw November 20, 2024 11:26
@tdyas
Copy link
Contributor Author

tdyas commented Nov 20, 2024

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

@huonw
Copy link
Contributor

huonw commented Nov 20, 2024

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

@tdyas
Copy link
Contributor Author

tdyas commented Nov 20, 2024

(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 tdyas changed the title [WIP] PyO3: upgrade to v0.23.x PyO3: upgrade to v0.23.x Nov 20, 2024
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for breaking up the commits.

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?

@tdyas
Copy link
Contributor Author

tdyas commented Nov 21, 2024

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
Copy link
Contributor Author

tdyas commented Nov 21, 2024

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

@tdyas tdyas merged commit 58aea21 into pantsbuild:main Nov 21, 2024
24 checks passed
@tdyas tdyas deleted the pyo3/upgrade-v0.23.x branch November 21, 2024 14:06
@huonw
Copy link
Contributor

huonw commented Nov 22, 2024

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants