-
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
Always call PyNumber_Index when casting from Python to a C++ integral type, also pre-3.8 #2801
Always call PyNumber_Index when casting from Python to a C++ integral type, also pre-3.8 #2801
Conversation
6c44020
to
657d7f6
Compare
include/pybind11/cast.h
Outdated
if (!tmp) { | ||
PyErr_Clear(); | ||
return false; | ||
} | ||
do_decref = true; | ||
obj = tmp; | ||
} | ||
#endif | ||
if (std::is_unsigned<py_type>::value) { | ||
py_value = as_unsigned<py_type>(obj.ptr()); |
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 might seems like a reasonably big change, but after this PR, I want to fix #2786, which involves a minor refactoring of casting to C++ integer types (to ensure future consistency with py::int_::operator int()
), so keep that in mind when reviewing, please ;-)
If we think that consistency between Python < 3.8 and >= 3.8 versions is a nice thing to have, then I personally don't really think this is a too high implementation price to pay.
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.
(Also, #2786's fix shouldn't be complex either, so if you're able to wait 1 or 2 more days, it can also still be a fix to go into 2.6.2. But we need to draw a line somewhere, ofc.)
657d7f6
to
2eb35ac
Compare
There was an issue in setuptools 51.3.0 fixed in 51.3.1, I've restarted the build. pypa/setuptools#2535 |
Thanks :-) I'd noticed setuptools had release suspiciously recently, but I didn't manage to figure out what was going in anymore, at 2 am. |
I'm mildly in favor. We don't want to rush a release out; after I get the PRs that were blocked by the GitHub API issue & CMake, I'd like @rwgk to run a global test before we pull the trigger on a release. So there's a bit more time. |
Will do. (I'm pretty neutral on this PR yes/no for v2.6.2.) |
include/pybind11/cast.h
Outdated
PyErr_Clear(); | ||
return false; | ||
} | ||
do_decref = true; |
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.
index_owner = reinterpret_steal<object>(tmp);
That way you don't need the second #if PY_VERSION_HEX < 0x03080000
below and this code become exception safe.
I'd also use idx
(or similar) instead of tmp
, to be more descriptive.
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 thought of/tried that, but didn't want to incur an overhead refcounting on Python >= 3.8, and this is also what CPython does.
But wait, maybe you mean something else, that doesn't need this! I'll give this a shot :-)
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.
Yesss, that does work out beautifully! Thanks!
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.
Still seeing if this could easily be refactored out.
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.
OK, it's quite hard to refactor into a separate private function without incurring an additional inc_ref
/dec_ref
, it seems. It's already cleaner than before, though, so is it fine to leave like this for now?
…rnings in >=3.8
Thanks @YannickJadoul, that looks great! I'll run this through our big testing system asap (probably tonight). |
Good, thanks! I'm pretty confident this PR does as it says (given our own tests and how they caught some corner cases), but it's good to have an idea how this "backported behavior" interacts in a larger context. |
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 looks fine to me.
I'm seeing 2 test failures that look related to this PR. |
In both failing tests a NumPy array with one
@YannickJadoul, is that description accurate? |
I think the current (in this PR) behavior is the correct one; a NumPy float should not be automatically converted to an int. If the they want to support floats, there should be a float function/method/constructor that does the conversion. NumPy no longer allows floats in indexing, as well. |
Huh, this is weird though. I thought this PR would only make things more permissive! Let me try out a few things. |
I agree with @henryiii and I'm OK if you want to merge for 2.6.2, although strictly speaking it's bending the rules to introduce this behavior change in a minor release, now that we know there are things that will break. |
Are you sure that current master without this PR doesn't also trigger that? Assuming it's the other PR that caused this? |
It's breaking something that shouldn't have worked, though, so it's a bit of a grey area. |
Yes, sure. Before attempting a fix, I commented out the `return false;`
after `PyNumberIndex`, and the `src_or_index = index;` assignment, which
made the test work.
…On Fri, Jan 22, 2021 at 9:55 AM Henry Schreiner ***@***.***> wrote:
Are you sure that current master without this PR doesn't also trigger
that? Assuming it's the other PR that caused this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2801 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFUZACDRCKRNLOWGP65LNLS3G32FANCNFSM4WFT3GAA>
.
|
Yeah, so |
4770ac1
to
cffc586
Compare
309e6a8
to
8356073
Compare
Thanks @YannickJadoul, I'll run this PR through our big old testing mill again, later tonight. FYI: In the meantime one team already merged my fix for them. (I'm still waiting for feedback on the second fix.) |
Oh, OK. I don't think they're even to blame, actually, since things did just work on 3.8. Thanks for finding this! I still need to figure out how to get 2.7 to at least pass tests. (Maybe not worth putting too much effort into 2.7.) |
The failures were actually useful. I don't think the code was working as intended by the original authors, only scraping by. |
Ah, good to know it was at least worth the effort, then! |
OK, got it. Python 2's |
I'll get back to this/fix this tomorrow, btw. |
fac0aa0
to
2092295
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.
OK, this fixes patches things, adding some more duct tape here and there.
Seems like refactoring/restructuring the float
/int
type caster is overdue, as well, but I propose to not cram that into this PR anymore.
Also, yes, this could use another test round, @rwgk. Who knows what I missed, this time.
Next to that test round, it's also worth checking: is this series of tests the behavior we want?
tests/test_builtin_casters.py
Outdated
@@ -286,14 +300,20 @@ def cant_convert(v): | |||
assert noconvert(7) == 7 | |||
cant_convert(3.14159) | |||
assert convert(DeepThought()) == 42 | |||
require_implicit(DeepThought()) | |||
requires_conversion(DeepThought()) | |||
assert convert(DoubleThought()) == 0 # Fishy; `int(DoubleThought)` == 42 |
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 not great, but kind of a consequence of us saying "everything with __index__
is already an int
, so don't try converting".
py_value = as_unsigned<py_type>(src_or_index.ptr()); | ||
} else { // signed integer: | ||
py_value = sizeof(T) <= sizeof(long) | ||
? (py_type) PyLong_AsLong(src_or_index.ptr()) |
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.
Oh, another note: this results in warnings, and I think that's correct. Because it's not just getting out the long
from the PyLong
object, but it's also trying to do the conversion.
Yes, the C API is quite messy here. And it's only made worse by the structure of this caster.
FYI: I tried running this through our global testing last night but something went wrong, the tests only ran with a previous version of this PR. I'll try again. |
To keep track of an observation, below is the error I saw in the first global testing run. With the current version of this PR the same test passes. For completeness: the dreamplace code used here is ~1 year behind github.
|
This should now also be covered by our own tests. I added the |
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 PR passed the Google global testing now.
Thanks @YannickJadoul!
tests/test_builtin_casters.py
Outdated
@@ -256,6 +256,13 @@ class DeepThought(object): | |||
def __int__(self): | |||
return 42 | |||
|
|||
class DoubleThought(object): |
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.
Double meaning it has index and int?
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, that's what I meant :-) As admitted yesterday:
yes, this naming joke got out of hand quite quickly
Should I still pick better names?
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. IntIndexThought, at least. :)
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.
Yes. IntIndexThought, at least. :)
I liked the mix of HHGG and 1984, but yes ;-)
LGTM! |
Thanks, all! Another tiny bit of progress :-) |
Description
See the remaining decision to be made in #2698: #2698 (comment)
In one sentence, this makes the casting from Python objects to C++ integer types consistent across Python versions, "backporting" Python 3.8 behavior on handling
__index__
to pre-3.8 Pythons.As @bstaletic pointed out to me, we have done this before, most often backporting Python 3 behavior to Python 2 (unicode, for example), but also in #2616, "backporting" 3.8 behavior.
Given the original issue fixed by #2698, and especially the discussion following that ("everything with
__index__
is an integer type"), I think this is the logical to minimize surprises when developing pybind11 libraries. See also the changed test demonstrating how things are more consistent across versions.I'm cheekily adding this to 2.6.2, because in my eyes, it's still part of #2698, and I'd like to see a decision made ("no, because ..." is also fine, btw; then we close this PR! But it doesn't make sense to me to delay this simple decision on policy.). This PR mainly makes it easier to judge the changes (after soon merging #2698, at least).
Suggested changelog entry:
When casting to a C++ integer, `__index__` is always called and not considered as conversion, consistent with Python 3.8+.