-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Updated STL casters and py::buffer to use collections.abc #5566
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
The failing check is unrelated, I think (maybe rerun is enough). |
After seeing #5498 and digging deeper into the caster code, I noticed that I have to think more about this. These three functions restrict the casters further than I thought: pybind11/include/pybind11/stl.h Lines 88 to 119 in 2e382c0
For example, it requires the mapping to be of type or subtype of set or frozenset or have dict_keys if I understand correctly.The caster itself uses the Mapping protocol though and could easily be changed to fully allow it. I will add some tests to better map out what is allowed and what not and how this relates to the type hints. Git blame directed me to #4686, which seems to have more insight. |
@timohl Did you see these already?
I'm fine if you want to work on those functions. I'm thinking it's best to keep the current logic, which is super fast, but where we're currently returning |
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 only glanced through very quickly. Is this still in draft mode intentionally?
I would like to improve the comments before finalizing and being ready to merge. Also, your comment about the function names sounds good. I will change that. |
No rush, at all, from my end. I just wanted to be sure you're not waiting for my feedback. |
Would it also be possible to update |
Going through the code here: pybind11/include/pybind11/cast.h Lines 1289 to 1392 in f365314
There are a bunch of other types that could be changed:
|
Currently stub generators typically know that |
I have changed The convertible check functions now contain some comments to make it more obvious what is allowed and what not. I removed explicit checks for methods required by the class FakeSeq(collections.abc.Sequence): ...
a = FakeSeq()
|
I have just updated the PR description as well. |
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 production code is great as-is! I only have suggestions for simple changes to the test code.
Here is another small suggestion generated by ChatGPT:
The PR includes comments explaining the rationale behind the changes, particularly the shift to using
collections.abc
. These comments are clear and provide valuable context. However, ensuring consistency in terminology (e.g., consistently referring tocollections.abc.Set
rather than alternating withcollections.Set
) would enhance clarity.
tests/test_stl.py
Outdated
"a": 1, | ||
"b": 2, | ||
"c": 3, | ||
} |
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.
Might be nice to keep this more compact via
a1b2c3 = {"a": 1, "b": 2, "c": 3}
and then reuse three times.
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 added some commits to address your comments.
I am not sure though what chatgpt means here. |
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 am not sure though what chatgpt means here.
Searching for the term"collections"
in the entire code base, I could not find any inconsistency in terminology.
Eithercollections.abc
is used correctly or the term is used in another context as far as I can see.
Sorry, I didn't double-check the ChatGPT finding; must be a hallucination then. Thanks for checking!
@timohl @InvincibleRMC @rwgk |
Tested in Python 3.8: >>> import collections.abc
>>> def b(x: collections.abc.Buffer) -> float:
... return 4
...
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: module 'collections.abc' has no attribute 'Buffer'
>>> from __future__ import annotations
>>> def b(x: collections.abc.Buffer) -> float:
... return 4
...
>>> Stubs generated by Do you have a use case or tool where this creates a problem? |
You are correct that there are no runtime issues but a static type checker in 3.11 would error because |
Ok, I see. Pylance and mypy are complaining, that is true. nanobind uses a central place to define typing types depending on the Python version: Maybe something similar would be good instead of having version checks all over the place. |
My personal preference would be to keep it where it gets used unless it is used in multiple places but I am not a developer here. |
Description
This updates type hints for the
set
,map
,list
andarray
STL casters, as well as forpy::buffer
, to use the more genericcollections.abc
types in convertible arguments.The casters have been changed to allow types derived from those abstract base classes.
set_caster
collections.abc.Set
set
map_caster
collections.abc.Mapping
dict
list_caster
collections.abc.Sequence
list
array_caster
collections.abc.Sequence
list
Old description:
During testing I found that
map_caster
andset_caster
did not always allow for types derived fromcollections.abc.Mapping
andcollections.abc.Set
since the checks contained additional restictions (like having anitems()
method for mappings), while the list caster already allowed types derived fromcollections.abc.Sequence
.This PR changes the behavior of those casters to allow types derived from those base classes.
Additionally, the
array_caster
was updated to match thetyping.Annotated
style of numpy/eigen type hints.As suggested by @InvincibleRMC, the type hint
Buffer
was changed tocollections.abc.Buffer
sinceBuffer
does not exist in thetyping
module.Resolves #5498
Suggested changelog entry: