-
Notifications
You must be signed in to change notification settings - Fork 880
Fetch type name dynamically on cast errors instead of using PyTypeInfo::NAME
#5387
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
Conversation
CodSpeed Performance ReportMerging #5387 will degrade performances by 10.84%Comparing 🎉 Hooray!
|
Benchmark | BASE |
HEAD |
Change | |
---|---|---|---|---|
❌ | vec_bytes_from_py_bytearray_small |
770 ns | 863.6 ns | -10.84% |
c459414
to
7b921c9
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 for moving this forward. I wonder if we should be concerned by the performance hit? It's not surprising as we're moving work from compile-time to runtime, at the benefit of better accuracy. I wonder if there are ways we can have our cake and eat it too? 🤔
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, looks very promising! I think there might be a couple other locations to change (see other comment)
... this might lead to some error messages changing in tests.
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 there are other callsites to change in this file
cast
andcast_into
Borrowed::cast
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.
Sadly it won't work because they use the PyTypeCheck
trait and not the PyTypeInfo
trait
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 had a quick look at the types implementing PyTypeCheck
and not PyTypeInfo
. It's
- datetime types when abi3 is enabled. This can be fixed easily: DateTime: implement PytypeInfo even using the stable ABI #5388
- protocol-like types (PyCode, PyIterator, PyMapping, PySequence, PyWeakref, PyWeakrefProxy and PyWeakrefReference). My guess is that we might implement
PyTypeInfo
on them, returning thecollections.abs
or similar types intype_object_raw
impl and using the fast C method inis_type_of
What do you think about it?
Aside: rust-numpy does not implement PyTypeCheck
explicitly.
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.
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.
Added PyTypeCheck::classinfo_object
that provides a workaround
ce7a0f6
to
196ed31
Compare
impl<'a, 'py> DowncastError<'a, 'py> { | ||
/// Create a new `PyDowncastError` representing a failure to convert the object | ||
/// `from` into the type named in `to`. | ||
pub fn new(from: &'a Bound<'py, PyAny>, to: impl Into<Cow<'static, str>>) -> Self { |
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.
DowncastError::new
and DowncastIntoError::new
are unused now. Do we want to replace the second parameter with a Bound<'py, PyAny>
?
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 we should leave this for the moment, as public API. We might want to re-name / re-introduce this as CastError
, and if so, we could "fix" the API and deprecate this one at the same time.
062a32f
to
dd0d5c5
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 is looking great, the improved error messages make me very happy! I think let's just make this land slightly softer for users with a deprecation, I don't think we need the immediate removal.
/// Name of self. This is used in error messages, for example. | ||
const NAME: &'static str; |
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.
Can we make this #[deprecated]
instead of removing? The deprecation message can recommend users get a "name" at runtime from classinfo_object
, perhaps.
(We might be able to default it to empty, so that users don't need to implement it, we might want to leave existing implementations in PyO3 for a couple of versions just in case users are depending on it.)
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.
Done in 6a9135f. This is indeed used by a few repos on GitHub. I added a longer deprecation message because most of them are doing their own version of cast
.
With this MR all remaining usages of |
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.
Fantastic, thank you!
6a9135f
to
f5fbe62
Compare
@davidhewitt Thank you! f5fbe62 should fix the CI (the type name is different in 3.9) |
Head branch was pushed to by a user without write access
Rebase done: 1963b29 |
# Conflicts: # src/types/sequence.rs
1963b29
to
01d11e1
Compare
Introduce
PyTypeCheck::classinfo_object
that returns an object compatible withinstance
and make use of it.Remove
PyTypeCheck::NAME