Support frozenset, tuple as dict keys #3886

ecatmur · 2022-04-19T17:35:45Z

Description

Add frozenset as a pybind11 type.
Allow type_caster to be (explicitly) selected and specialized, defaulting to (inherit from) type_caster.
Use type_caster for std::set, std::map etc. key converter.
Specialize type_caster for std::set, std::vector etc.
Add tests.

Suggested changelog entry:

std::map<std::set<int>, int> now converts to/from Dict[FrozenSet[int], int]; likewise std::set<std::vector<int>> to/from Set[Tuple[int, ...]].

pybind#3836 Add frozenset as a pybind11 type. Allow type_caster<const T> to be (explicitly) selected and specialized, defaulting to (inherit from) type_caster<T>. Use type_caster<const K> for std::set, std::map etc. key converter. Specialize type_caster<const C> for std::set, std::vector etc. Add tests.

for more information, see https://pre-commit.ci

…frozenset

Skylion007 · 2022-04-19T19:15:21Z

The frozen set casters are a decent idea, but I don't know how I feel about the tuple ones considering we have std::tuple to fulfill that role already. Thoughts @rwgk @henryiii ?

Skylion007 · 2022-04-19T19:15:34Z

The frozen set casters are a decent idea, but I don't know how I feel about the tuple ones considering we have std::tuple to fulfill that role already. Thoughts @rwgk @henryiii ?

Skylion007 · 2022-04-19T19:13:13Z

include/pybind11/pytypes.h

@@ -1784,14 +1784,11 @@ class kwargs : public dict {
    PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check)
 };

-class set : public object {
+class set_base : public object {
+protected:


I don't like having add as a public function on frozen-sets. We can still call the C-API directly on the pointer for casters, but other code shouldn't be able to add objects to frozen sets.

moved non-const methods to protected

They would still need to be public for normal set?

@ecatmur Why not just move them to set subclass and use the raw C-API to interact with the frozen set.

Sorry, I get it now. Fixed.

s.t. they aren't public on frozenset

This reverts commit a56f91c.

This reverts commit 6c045a5.

call PySet_Add directly in caster

for more information, see https://pre-commit.ci

ecatmur · 2022-04-20T00:24:36Z

The frozen set casters are a decent idea, but I don't know how I feel about the tuple ones considering we have std::tuple to fulfill that role already.

I think the problem is that historically (and currently) Python's tuple has a bit of a mixed role: it's the container you go for when you need an immutable homogeneous sequence, as well as being the heterogeneous sequence type. For example, in typing you can write Tuple[int, ...] as well as Tuple[int, str].

FWIW, when PEP 351 was rejected (over 15 years ago!) Raymond Hettinger made the argument that list->tuple and set->frozenset are the only two transformations you actually ever need: https://mail.python.org/pipermail/python-dev/2005-October/057586.html That suggests a simpler approach would be to adjust list->tuple and set->frozenset after casting before insertion into dict (or set), avoiding the hairiness of type_caster<type const> and transparently supports all types casting to list and set (including third party stuff) at a small runtime cost. I'll propose that as an alternative.

jiwaszki

@Skylion007 @ecatmur I will join as the interested party. Thanks for porting my draft into actual codebase!

I tried the branch and get multiple errors during compilation on my Mac:

error: ambiguous partial specializations of 'type_caster' const_name("FrozenSet[", "Set[") + key_conv::name

Thus I was unable to test it further, however I have some observations.

Personally I do not support an idea of the implicit cast from non-const std::set to frozenset. As I mentioned in Convert std::set to frozenset #3836 -- there should be a clear line between set and it's immutable version as it might confuse Python/pybind users. Ideally functions should only accept dicts coming (or getting out) in form of std::map<const std::set<...>, ...>. Is there a way to restrict that?
I have also noticed that tests lack some of essential scenarios. What happens in case when code mix const and non-cost types? What happens when arguments are passed by copy?
Following @Skylion007 concern about tuples as I have similar thoughts. std::tuple is already filling that niche, there is no need to expand in that area. As Python tuple will be converted to std::vector it will not only allow appending, but actually allow to modify it's contents -- I do not think this is an expected behaviour. If a user wants to do it, just use py::tuple and manually transfer the data to the mutable container.

jiwaszki · 2022-04-20T08:37:39Z

tests/test_stl.py

@@ -94,6 +94,38 @@ def test_recursive_casting():
    assert z[0].value == 7 and z[1].value == 42


+def test_frozen_key(doc):


I see that you added tests for each container, what about frozenset as standalone type? Is it even possible?

Added fcaff44 that tests you can pass frozenset to a C++ function that takes std::set (just as you can pass tuple to a C++ function taking std::vector). Is there's anything else you can do with frozenset on its own?

for more information, see https://pre-commit.ci

…frozenset

ecatmur · 2022-04-21T01:35:10Z

I tried the branch and get multiple errors during compilation on my Mac:

error: ambiguous partial specializations of 'type_caster' const_name("FrozenSet[", "Set[") + key_conv::name

Oh, sorry. Maybe #3887 would work? It's a simpler approach but the observable behavior should be the same.

Thus I was unable to test it further, however I have some observations.

Personally I do not support an idea of the implicit cast from non-const std::set to frozenset. As I mentioned in Convert std::set to frozenset #3836 -- there should be a clear line between set and it's immutable version as it might confuse Python/pybind users. Ideally functions should only accept dicts coming (or getting out) in form of std::map<const std::set<...>, ...>. Is there a way to restrict that?

There isn't really such a thing as std::map<const std::set<...>, ...> - it isn't possible to get a non-const reference to the key in a std::map (not without an illegal const_cast, anyway) so no-one would bother putting const on the key. The value_type is std::pair<const Key, Value> so the keys are const as long as they're in the map.

So there isn't a conversion from non-const std::set to frozenset, it's from const std::set to frozenset.

I have also noticed that tests lack some of essential scenarios. What happens in case when code mix const and non-cost types? What happens when arguments are passed by copy?

I'm only implementing this for the caster interface, not the binder interface, so everything is being passed by copy already and there's no difference between const and non-const at the top level.

Following @Skylion007 concern about tuples as I have similar thoughts. std::tuple is already filling that niche, there is no need to expand in that area. As Python tuple will be converted to std::vector it will not only allow appending, but actually allow to modify it's contents -- I do not think this is an expected behaviour. If a user wants to do it, just use py::tuple and manually transfer the data to the mutable container.

We already allow passing tuple to C++ functions that take a std::vector (etc.) - there's one or two tests that show that already, and I've just added some more.

If Python code inserts a tuple key into a dict[tuple[str, ...], int] and passes it to C++ code as a std::map<std::vector<std::string>, int> then the C++ code can't modify the std::vectors (since they're const within the map) and if it removes them, inserts modified keys and passes the map back to Python then they'll be completely distinct objects. There's no way for a std::vector to alias a tuple and allows modifying it.

rwgk · 2022-04-23T01:29:44Z

I looked up and down both PRs quite a bit, but didn't get a chance yet to fully appreciate all details. It looks very interesting: not a lot of code, but makes something work that currently doesn't.

Which of the PRs do you prefer? I'd like to focus on the one that you think is best.

Another idea: could you please transfer the additional tests for tuple and frozenset that already work on current HEAD to a separate PR, even if it's a tiny PR? That will make it more clear here what already works and what doesn't. The more immediately improved test coverage is a nice plus.

We already test that tuple can cast to std::vector and std::deque; add tests for std::vector<bool>, std::list and std::valarray. Extracted from pybind#3886.

For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector but std::vector casts to list. Extracted from pybind#3886.

since this will be part of pybind11 public API

ecatmur · 2022-04-24T21:22:57Z

I looked up and down both PRs quite a bit, but didn't get a chance yet to fully appreciate all details. It looks very interesting: not a lot of code, but makes something work that currently doesn't.

Which of the PRs do you prefer? I'd like to focus on the one that you think is best.

#3887. While this one (#3886) is more idiomatic C++, #3887 is more Pythonic and that's appropriate for code that affects the Python side of things - it's automatically creating a Python API, so should stick to types that would appear naturally in Python programs. The clever recursive stuff would work in the STL binder API, perhaps.

Another idea: could you please transfer the additional tests for tuple and frozenset that already work on current HEAD to a separate PR, even if it's a tiny PR? That will make it more clear here what already works and what doesn't. The more immediately improved test coverage is a nice plus.

Sure: #3900 is for tuple tests. #3901 splits out the core frozenset functionality (any_set as a common base type, and std::set cast from any_set).

We already test that tuple can cast to std::vector and std::deque; add tests for std::vector<bool>, std::list and std::valarray. Extracted from #3886.

Skylion007 · 2022-04-27T15:08:57Z

Let's focus on merging #3901 for now.

and rename anyset methods

Making frozenset non-default constructible means that we need to adjust pyobject_caster to not require that its value is default constructible, by initializing value to a nil handle. This also allows writing C++ functions taking anyset, and is arguably a performance improvement, since there is no need to allocate an object that will just be replaced by load. Add some more tests, including anyset::empty, anyset::size, set::add and set::clear.

for more information, see https://pre-commit.ci

* Add frozenset, and allow it cast to std::set For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector but std::vector casts to list. Extracted from #3886. * Rename set_base to any_set to match Python C API since this will be part of pybind11 public API * PR: static_cast, anyset * Add tests for frozenset and rename anyset methods * Remove frozenset default ctor, add tests Making frozenset non-default constructible means that we need to adjust pyobject_caster to not require that its value is default constructible, by initializing value to a nil handle. This also allows writing C++ functions taking anyset, and is arguably a performance improvement, since there is no need to allocate an object that will just be replaced by load. Add some more tests, including anyset::empty, anyset::size, set::add and set::clear. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add rationale to `pyobject_caster` default ctor * Remove ineffectual protected: access control Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

ecatmur and others added 4 commits April 19, 2022 18:26

[pre-commit.ci] auto fixes from pre-commit.com hooks

d0f9f2e

for more information, see https://pre-commit.ci

Fix for C++11 mode

038904a

Merge branch 'frozenset' of https://github.com/ecatmur/pybind11 into …

27986dd

…frozenset

Skylion007 requested review from rwgk and henryiii April 19, 2022 19:15

Skylion007 reviewed Apr 19, 2022

View reviewed changes

ecatmur and others added 6 commits April 19, 2022 21:39

protect non-const methods

a56f91c

s.t. they aren't public on frozenset

formatting

cd09b3a

Revert "protect non-const methods"

6c045a5

This reverts commit a56f91c.

Revert "Revert "protect non-const methods""

bb123d1

This reverts commit 6c045a5.

Move add() and clear() to set

6b55983

call PySet_Add directly in caster

[pre-commit.ci] auto fixes from pre-commit.com hooks

3eb1a8b

for more information, see https://pre-commit.ci

ecatmur mentioned this pull request Apr 20, 2022

Support frozenset, tuple as dict keys (alternative approach) #3887

Open

jiwaszki reviewed Apr 20, 2022

View reviewed changes

ecatmur and others added 4 commits April 21, 2022 02:08

Only use const type_caster for class types

1b7a941

[pre-commit.ci] auto fixes from pre-commit.com hooks

0ebef3b

for more information, see https://pre-commit.ci

More tests for tuple -> list, frozenset -> set

fcaff44

Merge branch 'frozenset' of https://github.com/ecatmur/pybind11 into …

2096750

…frozenset

ecatmur added 2 commits April 24, 2022 21:54

Add frozenset, and allow it cast to std::set

e828031

For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector but std::vector casts to list. Extracted from pybind#3886.

Rename set_base to any_set to match Python C API

f2db7bb

since this will be part of pybind11 public API

This was referenced Apr 24, 2022

Add tests for cast from tuple to sequence containers #3900

Merged

Add anyset & frozenset, enable copying (cast) to std::set #3901

Merged

rwgk pushed a commit that referenced this pull request Apr 24, 2022

Add tests for cast from tuple to sequence containers (#3900)

9bc2704

We already test that tuple can cast to std::vector and std::deque; add tests for std::vector<bool>, std::list and std::valarray. Extracted from #3886.

ecatmur and others added 6 commits May 1, 2022 12:17

PR: static_cast, anyset

ef92aa5

Add tests for frozenset

736f293

and rename anyset methods

[pre-commit.ci] auto fixes from pre-commit.com hooks

05b6147

for more information, see https://pre-commit.ci

Merge remote-tracking branch 'upstream/master' into frozenset-core

26a29f4

Merge branch 'frozenset-core' into frozenset

0fb3a4f

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support frozenset, tuple as dict keys #3886

Support frozenset, tuple as dict keys #3886

ecatmur commented Apr 19, 2022 •

edited

Loading

Skylion007 commented Apr 19, 2022

Skylion007 commented Apr 19, 2022

Skylion007 Apr 19, 2022

ecatmur Apr 19, 2022

Skylion007 Apr 19, 2022

Skylion007 Apr 19, 2022

ecatmur Apr 19, 2022

ecatmur commented Apr 20, 2022

jiwaszki left a comment •

edited

Loading

jiwaszki Apr 20, 2022

ecatmur Apr 21, 2022

ecatmur commented Apr 21, 2022

rwgk commented Apr 23, 2022

ecatmur commented Apr 24, 2022

Skylion007 commented Apr 27, 2022

		@@ -94,6 +94,38 @@ def test_recursive_casting():
		assert z[0].value == 7 and z[1].value == 42


		def test_frozen_key(doc):

Support frozenset, tuple as dict keys #3886

Are you sure you want to change the base?

Support frozenset, tuple as dict keys #3886

Conversation

ecatmur commented Apr 19, 2022 • edited Loading

Description

Suggested changelog entry:

Skylion007 commented Apr 19, 2022

Skylion007 commented Apr 19, 2022

Skylion007 Apr 19, 2022

Choose a reason for hiding this comment

ecatmur Apr 19, 2022

Choose a reason for hiding this comment

Skylion007 Apr 19, 2022

Choose a reason for hiding this comment

Skylion007 Apr 19, 2022

Choose a reason for hiding this comment

ecatmur Apr 19, 2022

Choose a reason for hiding this comment

ecatmur commented Apr 20, 2022

jiwaszki left a comment • edited Loading

Choose a reason for hiding this comment

jiwaszki Apr 20, 2022

Choose a reason for hiding this comment

ecatmur Apr 21, 2022

Choose a reason for hiding this comment

ecatmur commented Apr 21, 2022

rwgk commented Apr 23, 2022

ecatmur commented Apr 24, 2022

Skylion007 commented Apr 27, 2022

ecatmur commented Apr 19, 2022 •

edited

Loading

jiwaszki left a comment •

edited

Loading