-
Notifications
You must be signed in to change notification settings - Fork 758
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
Conversation
Thanks for getting on with the first two points already.
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... |
Sure, I opened #4660 with that. |
d3b0667
to
80c92d4
Compare
80c92d4
to
f9d39e4
Compare
f9d39e4
to
ff7cb80
Compare
There was a problem hiding this 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.
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()), |
There was a problem hiding this comment.
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.
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into_any()), | |
CompareOp::Eq => Ok((self.0 == other.0).into_pyobject(py)?.into()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
There was a problem hiding this 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 anIntoPyObjectExt
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.
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()), |
There was a problem hiding this comment.
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.
CodSpeed Performance ReportMerging #4618 will improve performances by 15.84%Comparing Summary
Benchmarks breakdown
|
This deprecates
IntoPy
in favor ofIntoPyObject
. There are 3 blockers left:IntoPy::type_output
fromfeature = "experimental-inspect"
? Do we just transfer it ontoIntoPyObject
? moveIntoPy::type_output
toIntoPyObject::type_output
#4657IntoPy
for the vectorcall enhancement of Use vectorcall (where possible) when calling Python functions #4456. The plan was to split this out into somePyCallArgs
trait. Maybe we want to target 0.24 for that and revert to the simple approach for now? migratecall
API toIntoPyObject
#4653PyErrArguments
has a blanket forIntoPy<PyAny>
which we probably should migrate toIntoPyObject
. Its method should probably become fallible, but maybe we can justunwrap
there for now and tackle this in a later release (maybe along side with "lazy" state ofPyErr
... has challenges #4584) switchPyErrArguments
blanket impl toIntoPyObject
#4660