-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Only allow integer type_caster to call __int__ method when conversion is allowed; always call __index__ #2698
Conversation
Milestone? |
I'd consider this a fix; do you agree, @henryiii? If not, feel free to change to 2.7.0 |
Observation only, no analysis yet: This PR breaks https://github.com/google/mediapipe/blob/master/mediapipe/python/packet_test.py
|
Therefore, having an |
Main conclusion: this PR is working as intended, the mediapipe code needs to remove My feeling about milestone: given that we now know this PR breaks someone, maybe 2.7 is better? Might be nice to include the extra test below in this PR. It's similar to Tangential:
|
The problem is that both are treated equally by I feel stupid for not documenting this better, but I believe this was tackling a complaint from Gitter, where the conversion from So this was also why I considered this as a bugfix.
But @rwgk's comment also makes sense, from that respect, yes. Though ... well, it's very tricky behavior to depend on, I'd say?
I'll have a look at the tests as well, but in principle, these NumPy types are just as well just types with or without |
Yes, that's true, if you assume numpy behaves that way (seems like a very safe assumption). |
Those numpy types do simply implement |
Does that need to be updated or a different check used? There should be a difference between a class with only |
I think this is a 2.7 fix then, and I also still think |
…h np.intc has an __index__ method
I fixed the
Let's see why Python 2.7 is unhappy. sigh |
tests/test_builtin_casters.py
Outdated
assert convert(np.intc(42)) == 42 | ||
assert noconvert(np.intc(42)) == 42 | ||
|
||
assert convert(np.float32(3.14159)) == 3 # This might be wrong/unwanted? |
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.
This is also kind of weird. It's because we only explicitly test for float
and not for "float-like" types. I'm not sure if there's a general way to recognize "float-like" types, though. We could just add a special case for NumPy float types?
Not really related to this PR, though, so definitely another PR.
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 agree, although it looks tricky to implement, therefore I wonder about cost/benefit of working on this.
Pragmatic idea: report the situation in a comment and wait for an opportune moment in the future (e.g. when there is an actual problem that needs to be resolved). My first stab:
# The implicit conversion from np.float is undesirable but difficult to detect, without making the pybind11
# int converter dependent on numpy. Currently Python does not have a generalized concept of float-like types.
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.
We do already have something like this (a special case for NumPy) in the type_caster
for bool, though.
But yes, this might be something we'll have to live with? At the very least, this should now be considered a "conversion".
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.
The Google-global testing of this PR had no issues.
I see two behavior changes here: 1. the one currently implemented, and 2. potentially also raising a TypeError for np.float types (with ifdefs I guess to use the numpy api if available).
Given that this PR is exhaustively tested and doesn't disturb anything anymore, I feel it's best to merge now (for 2.6.2), and try the 2. behavior change in a separate PR. The main rationale for this two-stage approach is to use our time efficiently. Putting off this well-tested change will cost extra time.
For the comment, this is my 2nd suggested version:
# The implicit conversion from np.float is undesirable but currently accepted.
Just a hint that we thought about it, leaving all options open for the future.
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.
Agreed. Never meant to lump this in this PR. I really thought it would just be a straightforward one, but once again, Python had some surprises.
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.
Btw, to make things more complicated, np.float
to int
isn't allowed, because np.float
is just a Python float
(I'm not joking; this is really the case). Anyway, I'll add 32
and commit this suggestion.
even deeper sigh Old-style classes... |
tests/test_builtin_casters.py
Outdated
cant_convert(ShallowThought()) | ||
cant_convert(FuzzyThought()) | ||
if not env.PY2: | ||
# I have no clue why __index__ is not picked up by Python 2's PyIndex_check |
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.
Too bad for Python 2? I don't know what went wrong, here...
This does work in Python 2.7, though:
Python 2.7.17 (default, Sep 30 2020, 13:38:04)
[GCC 7.5.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> x = list(range(50))
>>> class IndexedThought(object):
... def __index__(self):
... return 42
...
>>> x[IndexedThought()]
42
Ready for another review |
tests/test_builtin_casters.py
Outdated
assert convert(np.intc(42)) == 42 | ||
assert noconvert(np.intc(42)) == 42 | ||
|
||
assert convert(np.float32(3.14159)) == 3 # This might be wrong/unwanted? |
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 agree, although it looks tricky to implement, therefore I wonder about cost/benefit of working on this.
Pragmatic idea: report the situation in a comment and wait for an opportune moment in the future (e.g. when there is an actual problem that needs to be resolved). My first stab:
# The implicit conversion from np.float is undesirable but difficult to detect, without making the pybind11
# int converter dependent on numpy. Currently Python does not have a generalized concept of float-like types.
I need to look into these new failures, but, @rwgk, this might break a lot fewer tests, on your end? |
I think this should fix the tests (it does so on my machine anyway, where I had failed to run a 3 <= Python version < 3.8, before). Remaining issue: Python 3.8: >>> int(IndexedThought())
42
>>> list(range(100))[IndexedThought()]
42 Python 3.6: >>> int(IndexedThought())
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: int() argument must be a string, a bytes-like object or a number, not 'IndexedThought'
>>> list(range(100))[IndexedThought()]
42 Python 2.7: >>> int(IndexedThought())
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: int() argument must be a string or a number, not 'IndexedThought'
>>> list(range(100))[IndexedThought()]
42 So should we make this consistent, and always call Minor extra inconsistency: in versions < 3.6, we now check |
I quick observation: https://github.com/google/mediapipe/blob/master/mediapipe/python/packet_test.py passes with the current version of this PR (915423d). |
tests/test_builtin_casters.py
Outdated
assert convert(np.intc(42)) == 42 | ||
assert noconvert(np.intc(42)) == 42 | ||
|
||
assert convert(np.float32(3.14159)) == 3 # This might be wrong/unwanted? |
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.
The Google-global testing of this PR had no issues.
I see two behavior changes here: 1. the one currently implemented, and 2. potentially also raising a TypeError for np.float types (with ifdefs I guess to use the numpy api if available).
Given that this PR is exhaustively tested and doesn't disturb anything anymore, I feel it's best to merge now (for 2.6.2), and try the 2. behavior change in a separate PR. The main rationale for this two-stage approach is to use our time efficiently. Putting off this well-tested change will cost extra time.
For the comment, this is my 2nd suggested version:
# The implicit conversion from np.float is undesirable but currently accepted.
Just a hint that we thought about it, leaving all options open for the future.
Added back to 2.6.2, as @rwgk suggested; we can always switch back to 2.7 (or 2.6.3), if there's no time to get this fully reviewed and in. There's still the issue from #2698 (comment) that doesn't really feel nice to me. I'm wondering if we shouldn't strive for more consistency in pybind11, and adopt 3.8+ behavior. |
I've moved the last question to be decided to a more concrete PR, #2801, demonstrating the implementation is straightforward. All that's left is arguing why we do or don't want this. |
…eck (unnoticed because we currently don't have PyPy >= 3.8)
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.
Yes, the PyPy fix looks great to me. Thank @YannickJadoul!
Except that ICC isn't happy with this. It's hardly been a day... :-| |
Aren't you glad we added that check? 😉 |
I'm thrilled. Anyway, yeah, otherwise that other PR had to do it. I'm trying to get rid of that lambda, by means of a macro, I guess. Wouldn't know what else it could be the issue. Sorry for the mess. Things worked smoothly, locally. |
c47e21b
to
b942b62
Compare
Submitted as https://foss.heptapod.net/pypy/pypy/-/issues/3383 |
Oof, all green again 😅 |
Description
As the title says and the patch shows: currently,
PyLong_AsLong
/PyLong_AsLongLong
gets called independently ofconvert
in thetype_caster
for integral types. This seems inconsistent with other type casters.Suggested changelog entry: