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

deprecate IntoPy in favor or IntoPyObject #4618

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 13, 2024

This deprecates IntoPy in favor of IntoPyObject. There are 3 blockers left:

@davidhewitt
Copy link
Member

Thanks for getting on with the first two points already.

PyErrArguments has a blanket for IntoPy<PyAny> which we probably should migrate to IntoPyObject.

Agreed, I think we can just make that change and leave it infallible for now 👍

To avoid chance of racing, can I leave that PR for you to complete? I think it shouldn't conflict with the other error changes I'm working on in e.g. #4655 and later PRs...

@Icxolu
Copy link
Contributor Author

Icxolu commented Oct 26, 2024

To avoid chance of racing, can I leave that PR for you to complete?

Sure, I opened #4660 with that.

@Icxolu Icxolu force-pushed the deprecate/into-py branch from d3b0667 to 80c92d4 Compare October 26, 2024 20:17
@Icxolu Icxolu marked this pull request as ready for review October 26, 2024 20:32
@Icxolu Icxolu force-pushed the deprecate/into-py branch from 80c92d4 to f9d39e4 Compare October 26, 2024 20:53
@Icxolu Icxolu force-pushed the deprecate/into-py branch from f9d39e4 to ff7cb80 Compare October 26, 2024 20:57
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me!

I still see a lot of .map(BoundObject::into_any) and the like, I suspect we will add an IntoPyObjectExt trait with some methods like .into_pyresult_pyobject(). That is probably something to follow up after 0.23 at this point.

guide/src/class.md Outdated Show resolved Hide resolved
CompareOp::Eq => (self.0 == other.0).into_py(py),
CompareOp::Ne => (self.0 != other.0).into_py(py),
_ => py.NotImplemented(),
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into_any()),
Copy link
Member

Choose a reason for hiding this comment

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

Does this work to get to PyObject? It might be a touch simpler for users to understand.

Suggested change
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into_any()),
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into()),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, currently it does not.

Copy link
Member

Choose a reason for hiding this comment

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

I think interestingly #4593 might help here. But again an improvement for after 0.23

src/conversion.rs Show resolved Hide resolved
tests/test_buffer_protocol.rs Outdated Show resolved Hide resolved
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Copy link
Contributor Author

@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.

Thanks for the review 🚀

I still see a lot of .map(BoundObject::into_any) and the like, I suspect we will add an IntoPyObjectExt trait with some methods like .into_pyresult_pyobject(). That is probably something to follow up after 0.23 at this point.

Yeah, in generic code this comes up a lot. A IntoPyObjectExt could be nice if we can find good names for the methods. I agree that we can postpone this to after 0.23.

guide/src/class.md Outdated Show resolved Hide resolved
CompareOp::Eq => (self.0 == other.0).into_py(py),
CompareOp::Ne => (self.0 != other.0).into_py(py),
_ => py.NotImplemented(),
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into_any()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, currently it does not.

Copy link

codspeed-hq bot commented Oct 29, 2024

CodSpeed Performance Report

Merging #4618 will improve performances by 15.84%

Comparing Icxolu:deprecate/into-py (593a479) with main (2d3bdc0)

Summary

⚡ 5 improvements
✅ 78 untouched benchmarks

Benchmarks breakdown

Benchmark main Icxolu:deprecate/into-py Change
test_empty_class_init_py 15.3 µs 13.9 µs +10.15%
test_args_kwargs_py 21.1 µs 18.5 µs +14.27%
test_args_kwargs_rs 15.8 µs 13.6 µs +15.84%
test_simple_kwargs_py 22.3 µs 20 µs +11.82%
test_simple_kwargs_rs 21.6 µs 18.8 µs +14.76%

@Icxolu Icxolu added this pull request to the merge queue Oct 29, 2024
Merged via the queue into PyO3:main with commit ca2f639 Oct 29, 2024
43 of 44 checks passed
@Icxolu Icxolu deleted the deprecate/into-py branch October 29, 2024 19:50
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