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

migrate FromPyObject for Bound and Py to new APIs #3776

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

davidhewitt
Copy link
Member

This PR is two commits which refine FromPyObject for our smart pointers:

  • the first adjusts the bound of FromPyObject for both Bound<T> and Py<T> to just be T: PyTypeCheck, while migrating to extract_bound at the same time. This is loosely a follow-up from Add extract_bound method to FromPyObject #3706
  • the second implements PyFunctionArgument for &Bound<T>, allowing #[pyfunction] and #[pymethods] to accept &Bound<T>. I think it's desirable to allow this, and it's not possible to implement FromPyObject for &Bound<T> because while the backwards-compatible definition of FromPyObject for the GIL-ref API can't express that lifetime of the reference correctly. In the future we could potentially have FromPyObject<'a, 'py> for &'a Bound<'py, T> if we introduce a second lifetime to FromPyObject.

(I think that second lifetime is necessary if we also wanted FromPyObject<'a, 'py> for Borrowed<'a, 'py, T> too, but I'm less motivated for that and similarly would like to skip adding PyFunctionArgument for Borrowed<T> unless users are reporting it as a useful addition.)

Copy link

codspeed-hq bot commented Jan 29, 2024

CodSpeed Performance Report

Merging #3776 will improve performances by 11.44%

Comparing davidhewitt:bound-extract (a60c182) with main (d35a6a1)

Summary

⚡ 1 improvements
✅ 78 untouched benchmarks

Benchmarks breakdown

Benchmark main davidhewitt:bound-extract Change
not_a_list_via_downcast 270.6 ns 242.8 ns +11.44%

@davidhewitt
Copy link
Member Author

I'll push to add some test coverage to this.

@davidhewitt
Copy link
Member Author

Ok, pushed.

@davidhewitt
Copy link
Member Author

Updated this just to add a note to the migration guide about FromPyObject for &Bound<T> not existing right now.

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

The migration looks correct to me (from purely looking at the implementation, i dont have the overview (yet) to see, how this ties into the trait and macro system).
I agree that &Bound should be usable in function and method argument position. I cant see a way of changing FromPyObject in a backwards compatible way, which would allow this implementation, either. So using PyFunctionArgument here seems reasonable to me.

PyFunctionArgument will need the same treatment as FromPyObject, as in introducing extract_bound, but that can also be a separate PR I think.

@davidhewitt
Copy link
Member Author

Thanks! Let's merge this then, and I'll get on with splitting out the rest of the PRs which depend on it.

PyFunctionArgument will need the same treatment as FromPyObject, as in introducing extract_bound, but that can also be a separate PR I think.

As PyFunctionArgument is hidden inside the impl_ submodule I think we can change it without worrying about backwards compatibility. But yes, that's for another PR 👍

@davidhewitt davidhewitt added this pull request to the merge queue Feb 2, 2024
Merged via the queue into PyO3:main with commit 8f8d4d3 Feb 2, 2024
36 checks passed
@davidhewitt davidhewitt deleted the bound-extract branch February 2, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants