Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: make stl.h
list|set|map_caster
more user friendly. #4686feat: make stl.h
list|set|map_caster
more user friendly. #4686Changes from 18 commits
d2a36d9
7d30378
039d1b7
bdba857
ec50ca0
eeb59af
3631886
b4f767b
42e22d7
9647a92
1ed90cf
3cc034b
18c4153
e7330f2
da29bf5
1fa338e
6b5c780
9150a88
62fda81
4b25f74
3cfc95b
59509f2
903faac
4ab40d7
5f7c9d4
ccfeb02
6c1ec6a
cc4d69c
dacb827
3fc2d2a
ff0724f
be02291
0a79f55
ae95424
8aa81d7
4435ed0
9aca5e8
9dc7d7c
d9eeea8
9ac319b
b834767
d19a0ff
69a0da8
c86fa97
940e190
f7725e4
fa72918
a6b9a00
ac3ac4d
87ff92a
3ae34f3
d1ffe4f
2267e5c
a38d0bd
fbf41fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 requires explicit subclasses, rather than any object that conforms to
collections.abc.Sequence
? (Likewise forcollections.abc.Mapping
andcollections.abc.Set
below) The Pythonic way to do this would be to ask if the object has the methods required by these protocols, rather than checking explicit inheritance.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.
Specifically, from https://docs.python.org/3/library/collections.abc.html:
Sequence
:__getitem__
,__len__
,__contains__
,__iter__
,__reversed__
,index
, andcount
.Set
:__contains__
,__iter__
,__len__
,__le__
,__lt__
,__eq__
,__ne__
,__gt__
,__ge__
,__and__
,__or__
,__sub__
,__xor__
, andisdisjoint
.Mapping
:__getitem__
,__iter__
,__len__
,__contains__
,keys
,items
,values
,get
,__eq__
, and__ne__
.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.
In addition to what I wrote yesterday, there was actually another concern in the back of my mind: the current implementation is super fast, direct
strcmp
calls for 5 (vector
) of 1 (set
) very short string(s).In contrast,
hasattr
is far more complex and probably orders of magnitude slower, although I wouldn't really worry about it in the usual bigger context (it'll not even be a blip on the radar in most cases). The code extra code complexity is the bigger worry.So there is a code complexity and runtime overhead cost.
What is the benefit?
That brings me back to my "large-scale field experiment" experience hinted in the PR description. In a nutshell:
Current pybind11 is super restrictive.
Original PyCLIF was super permissive.
I had this huge pile of data from the wild contrasting the two. Based on that I could see very clearly that there is very little to gain from being more permissive than what this PR implements. IOW I don't think paying the code complexity and runtime overhead cost will help in any significant way.
Could you please let me know if you have a different opinion? — It will probably cost me a whole day worth of effort to implement and fully test (global testing) attribute inspection. We will also have to agree on what subsets of the attributes we actually want to test against. I expect that there will be some devils in the details.
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 having a short path (check inheritance first) could alleviate much of the speed problem - the slower "correct" check could be done if this is not a subclass of X for common X. Also, this list is somewhat arbitrary - you probably included things (like
dict_keys
?dict_items
?) based on usage (in PyCLIF), rather than making a comprehensive list of everything iterable in Python? Also, I believe checking the attributes on a class (you don't need to check them on the instance) is pretty fast if you avoid the descriptor protocol (typing.runtime_checkable
switched to this in 3.12 (since 3.2) which made it much faster).From a usage standpoint, implementing the Protocol specified by these ABC's is supposed to make your class work wherever the standard library classes work, so it seems ideal if this could follow that. Not required (and we could start with this if you really want to), but it seems like the correct thing to do.
This is concretely specified in
collections.abc
. No arguments needed.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. Which basically means: this PR plus follow-on work to make the check more permissive.
Do you think it's reasonable then to merge this PR, and do the fallback attribute inspection in a separate PR? — To be completely honest, I wouldn't want to prioritize this, for the reasons explained before. It a significant amount of work, extra code complexity, and I don't think very many people will ever notice the difference in practice. But if someone else wants to invest the effort, I'd be on board.
I see, sounds good to me, but from a purely practical viewpoint, assuming that most duck-typing only implements a subset of the attributes, insisting on the full set of attributes here probably means nobody will ever see a difference in practice, unless they are specifically looking to pass the checks 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.
We need to check the signature of items or that attrs is at least callable, right? Otherwise, it will try (and fail to convert a class with an items attr attached to it, right?
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.
Good point, I didn't consider this before. PyCLIF doesn't have function overloading, it does not matter there.
Adding the callable check is easy (done), but I don't know if there is a way to check if the callable can be called without arguments (without actually calling it).
The conditions under which the missing check for number of arguments could go wrong are very special:
__getitem__
(to passPyMapping_Check()
)items
anditems
must be callablestd::map
(or similar)std::map
conversion is tried firstOnly then will the desired overload will not be reached.
Even if we can check for the number of arguments,
.items()
could have some arbitrary return value and we'd fall over processing that.The only truly certain alternative is to not have
PyMapping_Check()
at all.From memory: not having the
PyMapping_Check()
+.items()
path would not be easy to accommodate. I think I'd have to make pretty intrusive changes to user code.On the other hand, having the requirement that an object with
.__getitem__()
&.items()
must implement these to be compatible with a Python mapping seems very natural (what most everybody would expect and not easily violate). What do you think?But in any event, adding the callable check is easy and I'm happy to keep that.
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 just discovered
PyMapping_Items()
: 4ab40d7Using that makes the "mapping" requirement more formal.
Our gating conditions are stronger than those for
PyMapping_Check()
, I think supporting Python mappings in this way is a great practical compromise, usefulness vs purity.