Add `.keys` and `.values` to bind_map by bmerry · Pull Request #3310 · pybind/pybind11

bmerry · 2021-09-27T08:00:39Z

Description

Both of these implement views (rather than just iterators), and .items
is also upgraded to a view. In practical terms, this allows a view to be
iterated multiple times and have its size taken, neither of which works
with an iterator.

The views implement __len__, __iter__, and the keys view implements
__contains__. Testing membership also works in item and value views
because Python falls back to iteration. This won't be optimal
for item values since it's linear rather than O(log n) or O(1), but I
didn't fancy trying to get all the corner cases to match Python
behaviour (tuple of wrong types, wrong length tuple, not a tuple etc).

Missing relative to Python dictionary views is __reversed__ (only
added to Python in 3.8). Implementing that could break code that binds
custom map classes which don't provide rbegin/rend (at least without
doing clever things with SFINAE), so I've not tried.

The size increase on my system is 131072 bytes, which is rather large
(5%) but also suspiciously round (2^17) and makes me suspect some
quantisation effect.

Suggested changelog entry:

Improve the classes generated by ``bind_map``:
- Change ``.items`` from an iterator to a dictionary view.
- Add ``.keys`` and ``.values`` (both dictionary views).
- Allow ``__contains__`` to take any object.

bmerry · 2021-09-27T08:04:26Z

CC @henryiii @rainwoodman since this is a follow-on to #3271 / #3293.

bmerry · 2021-09-27T08:10:44Z

I've realised that one limitation of the __contains__ implementation is that testing presence of an object of the wrong type will raise TypeError rather than returning false. That's consistent with what the map class itself currently does, but inconsistent with Python (even with type annotations: typeshed says that __contains__ takes an object rather than the key type). What do you think of adding a fallback implementation of __contains__ to both keys_view and the mapping class itself which returns false when given a general object?

henryiii · 2021-09-27T13:59:35Z

That sounds reasonable to me. You could do a cast and have one impl, or have two.

henryiii · 2021-09-27T14:02:02Z

Wait, what increases in size? This should only affect code that uses std_bind to bind maps? Are you just looking at the test size increase (which will be larger due to more tests)?

bmerry · 2021-09-27T15:13:54Z

Wait, what increases in size? This should only affect code that uses std_bind to bind maps? Are you just looking at the test size increase (which will be larger due to more tests)?

Yes, the shared library for the tests. There isn't actually any new code C++ code for tests (there is new code in the .py files to test the extra bindings).

henryiii · 2021-09-28T14:10:51Z

But only code that uses this increases in size (which the tests do), so personally, I think it's probably fine.

henryiii · 2021-09-30T01:49:34Z

Were you going to handle arbitrary inputs to __contains__? Say with a py::object argument that you then cast, and return false otherwise?

bmerry · 2021-09-30T07:11:20Z

Were you going to handle arbitrary inputs to contains? Say with a py::object argument that you then cast, and return false otherwise?

Thanks for reminding me. Done.

henryiii · 2021-09-30T14:15:21Z

Wouldn't this potentially be smaller in binary form and have simpler signatures if you just had a single function with a py::object argument that you then cast, and return false otherwise? What was the effect on the binary test size, by the way?

bmerry · 2021-09-30T15:19:05Z

Wouldn't this potentially be smaller in binary form and have simpler signatures if you just had a single function with a py::object argument that you then cast, and return false otherwise? What was the effect on the binary test size, by the way?

I'll take a look when I can - unfortunately I have some other commitments for the next week. I went with overloading because I couldn't see a way to do it in one function without a cast_error exception if the object isn't a KeyType, and using exceptions for non-error control flow is frowned on in C++. I'm also not familiar enough with the differences between casting in overload resolution versus py::cast to be sure they're equivalent (when it comes to things like converting versus non-converting casts).

henryiii · 2021-09-30T18:43:48Z

Okay, let's go with this unless someone wants to try something else. It could later be optimized if that does end up helping. The goal is to release 2.8 on or before Python 3.10 (Monday), and this is the one feature that would be nice to get in, since it builds on the existing ones. This technically was started before our feature freeze on Monday. @rwgk or @Skylion007 what do you think? We should also get one more Google test run with this in before releasing this weekend / Monday.

Skylion007 · 2021-09-30T18:46:31Z

On one hand, it was started before our feature freeze. On the other hand, this new functionality could add breaking functionality, @rwgk What does Google Testing say about this PR?

rwgk · 2021-09-30T18:51:36Z

On one hand, it was started before our feature freeze. On the other hand, this new functionality could add breaking functionality, @rwgk What does Google Testing say about this PR?

I support this PR and will run the Google testing asap (today, result late tonight or tomorrow morning).

Both of these implement views (rather than just iterators), and `.items` is also upgraded to a view. In practical terms, this allows a view to be iterated multiple times and have its size taken, neither of which works with an iterator. The views implement `__len__`, `__iter__`, and the keys view implements `__contains__`. Testing membership also works in item and value views because Python falls back to iteration. This won't be optimal for item values since it's linear rather than O(log n) or O(1), but I didn't fancy trying to get all the corner cases to match Python behaviour (tuple of wrong types, wrong length tuple, not a tuple etc). Missing relative to Python dictionary views is `__reversed__` (only added to Python in 3.8). Implementing that could break code that binds custom map classes which don't provide `rbegin`/`rend` (at least without doing clever things with SFINAE), so I've not tried. The size increase on my system is 131072 bytes, which is rather large (5%) but also suspiciously round (2^17) and makes me suspect some quantisation effect.

Add extra overload of `__contains__` (for both the map itself and KeysView) which takes an arbitrary object and returns false.

To keep clang-tidy happy.

…ironment).

rwgk · 2021-09-30T19:28:27Z

Internally Google has a special way of handling header files, which exposed this small oversight:

ERROR: /google/src/cloud/rwgk/pybind11_pr_testing/google3/third_party/pybind11/BUILD:36:11: Compiling third_party/pybind11/include/pybind11/stl_bind.h failed: (Exit 1) wrapped_clang failed: error executing command third_party/crosstool/v18/stable/toolchain/bin/wrapped_clang '-DSOABI="cpython-37m-x86_64-linux-gnu"' '-DABIFLAGS="m"' '-DMULTIARCH="x86_64-linux-gnu"' -iquote . -iquote ... (remaining 240 arguments skipped). [forge_remote_host=jatb4]
./third_party/pybind11/include/pybind11/stl_bind.h:638:5: error: use of undeclared identifier 'py'
py::class_<KeysView> keys_view(
^
... (many more) ...

Interactive testing (before submitting for global testing) passes after removing 5 py::. I pushed the change to this PR, CI is running. I also rebased on current master.

rwgk

Google tests look good. The code looks good to me, too. I support including this in the 2.8 release, but I think it's best to leave it up to @henryiii to merge.

henryiii · 2021-10-01T13:24:25Z

I think it's fine to go in. Let's view master as 2.8 RC. Full version will come out Monday.

Summary: `Map` data such as `shots` is exposed through view classes. We need to keep the map alive as long as some of its objects are alive in python. For this to happen, we need - make the views keep alive the map - make the shots keep alive the view A better solution will be to use pybind's own bindings for `unordered_map`. However, this is only complete in pybind 2.8 pybind/pybind11#3310 Differential Revision: D32401524 fbshipit-source-id: 3fc30e710a2151dbb5097de8fe685ddfa80df675

Summary: Pull Request resolved: #822 `Map` data such as `shots` is exposed through view classes. We need to keep the map alive as long as some of its objects are alive in python. For this to happen, we need - make the views keep alive the map - make the shots keep alive the view A better solution will be to use pybind's own bindings for `unordered_map`. However, this is only complete in pybind 2.8 pybind/pybind11#3310 Reviewed By: fabianschenk Differential Revision: D32401524 fbshipit-source-id: 126141a7f02abe27fd376ae63fc788e87565dc0e

themarpe · 2022-06-01T18:46:30Z

+ class_<KeysView> keys_view(
+ scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local));
+ class_<ValuesView> values_view(
+ scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local));
+ class_<ItemsView> items_view(
+ scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local));


This naming seems to produce "erroneous" names for stub generation and subsequent parsing by Pylance or mypy, etc... Also sphinx autodoc has issues parsing it:
reading sources... [ 40%] references/python WARNING: invalid signature for autoclass ('depthai::BoardConfig.ItemsView[GPIOMap]')
Thoughts on renaming to *View_{name}, with an underscore? Seems to work well afterwards.
Example
Before:
class GPIOMap: def __init__(self) -> None: ... def items(self) -> BoardConfig.ItemsView[GPIOMap]: ... def keys(self) -> BoardConfig.KeysView[GPIOMap]: ... def values(self) -> BoardConfig.ValuesView[GPIOMap]: ... def __bool__(self) -> bool: ... @overload def __contains__(self, arg0: int) -> bool: ... @overload def __contains__(self, arg0: object) -> bool: ... def __delitem__(self, arg0: int) -> None: ... def __getitem__(self, arg0: int) -> BoardConfig.GPIO: ... def __iter__(self) -> Iterator: ... def __len__(self) -> int: ... def __setitem__(self, arg0: int, arg1: BoardConfig.GPIO) -> None: ... class ItemsView[GPIOMap]: def __init__(self, *args, **kwargs) -> None: ... def __iter__(self) -> Iterator: ... def __len__(self) -> int: ... ...
After:
class GPIOMap: def __init__(self) -> None: ... def items(self) -> BoardConfig.ItemsView_GPIOMap: ... def keys(self) -> BoardConfig.KeysView_GPIOMap: ... def values(self) -> BoardConfig.ValuesView_GPIOMap: ... def __bool__(self) -> bool: ... @overload def __contains__(self, arg0: int) -> bool: ... @overload def __contains__(self, arg0: object) -> bool: ... def __delitem__(self, arg0: int) -> None: ... def __getitem__(self, arg0: int) -> BoardConfig.GPIO: ... def __iter__(self) -> Iterator: ... def __len__(self) -> int: ... def __setitem__(self, arg0: int, arg1: BoardConfig.GPIO) -> None: ... class ItemsView_GPIOMap: def __init__(self, *args, **kwargs) -> None: ... def __iter__(self) -> Iterator: ... def __len__(self) -> int: ... ...

@themarpe sounds good. Please add a PR.

@themarpe Actually, hold on. Why is it having any issues? It should be generating a typing.KeysView . Is there some other reason here why it is struggling to parse it correctly?

@Skylion007 Thanks - ~~will do so soon.~~
EDIT: Opened the PR
I think its mainly from the *View[name] naming. I think the square brackets gets at it. Works okay for mypy & Sphinx when renamed to something different

pybind/pybind11#3310 added keys() to maps

rainwoodman reviewed Sep 27, 2021
View reviewed changes

Comment threadinclude/pybind11/stl_bind.h

bmerry force-pushed the bind-map-keys-values branch from 571bff8 to a32db87Compare September 29, 2021 07:30

rainwoodman approved these changes Sep 29, 2021
View reviewed changes

henryiii approved these changes Sep 30, 2021
View reviewed changes

bmerryand others added 4 commits September 30, 2021 12:26

bind_map: support any object in __contains__
7582a27
Add extra overload of `__contains__` (for both the map itself and KeysView) which takes an arbitrary object and returns false.

Take py::object by const reference in __contains__
1823edf
To keep clang-tidy happy.

Removing stray py:: (detected via interactive testing in Google env…
9422f17
…ironment).

rwgk force-pushed the bind-map-keys-values branch from b03ce2a to 9422f17Compare September 30, 2021 19:27

rwgk approved these changes Oct 1, 2021
View reviewed changes

henryiii merged commit b3573ac into pybind:masterOct 1, 2021

github-actionsBot added the needs changelog Possibly needs a changelog entry label Oct 1, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Oct 4, 2021

jiwaszki mentioned this pull request Oct 28, 2021
Pszmel/bind infer request akuporos/openvino#51
Merged

paulinus mentioned this pull request Nov 15, 2021
fix: use map views ref counts mapillary/OpenSfM#822
Closed

themarpe reviewed Jun 1, 2022
View reviewed changes

themarpe mentioned this pull request Jun 2, 2022
STL map bind naming fix for Sphinx and Pylance #3985
Closed

leoetlino mentioned this pull request Dec 26, 2022
oead 1.2.5 & 1.2.5.post1- BCML issues zeldamods/oead#25
Closed

leoetlino added a commit to zeldamods/oead that referenced this pull request Dec 26, 2022
py: Remove custom definition of keys() as it exists in pybind11 now
2d77e4e
pybind/pybind11#3310 added keys() to maps

rwgk mentioned this pull request Feb 10, 2023
FWD pybind11 google/pybind11clif#3310
Closed

Uh oh!

Conversation

bmerry commented Sep 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

bmerry commented Sep 27, 2021

Uh oh!

bmerry commented Sep 27, 2021

Uh oh!

henryiii commented Sep 27, 2021

Uh oh!

henryiii commented Sep 27, 2021

Uh oh!

bmerry commented Sep 27, 2021

Uh oh!

Uh oh!

henryiii commented Sep 28, 2021

Uh oh!

henryiii commented Sep 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmerry commented Sep 30, 2021

Uh oh!

henryiii commented Sep 30, 2021

Uh oh!

bmerry commented Sep 30, 2021

Uh oh!

henryiii commented Sep 30, 2021

Uh oh!

Skylion007 commented Sep 30, 2021

Uh oh!

rwgk commented Sep 30, 2021

Uh oh!

rwgk commented Sep 30, 2021

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

henryiii commented Oct 1, 2021

Uh oh!

themarpeJun 1, 2022

Choose a reason for hiding this comment

Uh oh!

Skylion007Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

Skylion007Jun 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

themarpeJun 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bmerry commented Sep 27, 2021•
edited
Loading

henryiii commented Sep 30, 2021•
edited
Loading

Skylion007Jun 1, 2022•
edited
Loading

themarpeJun 2, 2022•
edited
Loading