pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.6k stars 2.09k forks source link

Why pybind11 generated enum type is not PEP 435 compatible. say, it doesn't have value attribute and it's not iterable. #2332

Open frank-xlj opened 4 years ago

frank-xlj commented 4 years ago

EDIT(eric): Deleted template text in body. Title describes issue sufficiently.

For reference: PEP 435

EDIT 2 (eric): See #253 per Ashley's mention.

bstaletic commented 4 years ago

Because it predates enum.Enum by a lot.

You can use .def() on py::enum_s and give them values() and __iter__() to get what you want.

frank-xlj commented 4 years ago

Thanks @bstaletic for the comments. So pybind11 doesn't have the plan to make enum type PEP-435 compatible to keep backward compatibility, right?

bstaletic commented 4 years ago

There are no backwards compatibility concerns here. For value, it's just a simple

.def("value", [](enum_type e) { return static_cast<int>(e); })

Or you can do it from python with int(e). This is quite a small addition, so maybe we should add it.

As for __iter__, python implements it by storing a dictionary of names to enum_values inside the enum. I don't know how to implement that efficiently (without allocating), which doesn't mean it's impossible. Inefficient implementation would only hurt those who do not need iteration over an enum. Again, users can implement these on their own.

frank-xlj commented 4 years ago

got it. Thanks.

YannickJadoul commented 4 years ago

Seems solved/answered?

EricCousineau-TRI commented 3 years ago

Coming back to this, I would love to have this in pybind11 proper!

I have written some schema-ish stuff (like Hydra, but trying to integrate something like nptyping), and it's a bit painful to have to explicitly check if an enum is really a pybind11 enum...

Additionally, I was playing with pyrealsense2, and I wanted to print out each entry of rs2.camera_info for rs2.device. Yes, I can iterate on dir() and do some simple name/type checking, but ewwww.

I plan to implement a generic function to provide the functionality that @YannickJadoul mentioned, but I would like to eventually upstream it.

May PR sometime in the next 1-2 months.

At a minimum, it will be documentation encoding Yannick's suggested workaround.

EricCousineau-TRI commented 3 years ago

Ah! Looks like it's already provided via __members__, so really all we need is something like: https://pybind11.readthedocs.io/en/stable/classes.html#enumerations-and-internal-types

def __iter__(self):
    return iter(self.__members__.values())

I think the issue that I want to check if it's a pybind11 enum still stands. I'll see if there's a way to hack in __isinstance__, but I may timebox that.

(This is assuming __members__ is efficient)

anntzer commented 3 years ago

FWIW I have been used the implementation at https://gist.github.com/anntzer/96f27c0b88634dbc61862d08bff46d10 to get "pythonic" enums with pybind11 (i.e., enums that actually inherit from enum.Enum). More usefully, this also allows enums to inherit from any of enum.IntEnum, enum.Flag, or enum.IntFlag, which have quite useful additional properties (e.g. nicer reprs, for flags).

It'd be nice to have something similar built into pybind11.

EricCousineau-TRI commented 3 years ago

Just filed #2739 which at least adds the .value attirbute.

@anntzer Thanks! The only caveat is that it requiring enum.Enum constrains the feature to Python 3.4+ given PEP 354, and we still support Python 2.7.

I took a glance at the implementation of Enum (in 3.6.9 for my system), and it looks like supporting stuff like __iter__, __len__, and subclass checks shouldn't be too hard (also valid for Python 2.7). https://github.com/python/cpython/blob/v3.6.9/Lib/enum.py

The main challenges will be:

(1) Ensure that py::metaclass(whatever_enum_metaclass) doesn't create segfaults, e.g. connecting to: https://github.com/pybind/pybind11/blob/v2.6.1/include/pybind11/detail/internals.h#L308-L309 (2) Try to minimize the footprint of whichever metaclass. I'll probably start off prototyping in pure Python (using py::exec), then lower it to whatevs CPython API :roll_eyes:

EricCousineau-TRI commented 3 years ago

See also: @AWHetter's PR in #2704 (inspired by @aldanor).

EricCousineau-TRI commented 3 years ago

Moving comment from https://github.com/pybind/pybind11/pull/2739#issuecomment-750496252 (design discussion ~not entirely relevant to~ relevant to the simplified PR, but perhaps shouldn't block it):


However, shouldn't we be looking at python3.4 enums, as in #2704 ?

Fancy! Didn't see that PR (should've searched lol) The main concern I have is ensuring we maintain the same semantics as what we currently have - i.e. strict increase in API / capability. The solution space from @AWhetter / @aldanor / @anntzer entail doing a sort-of stl_bind.h / macro-type_caster approach. That's totes an option, but I'm concerned about how much it deviates from the existing RTTI-based registration based in py::class_ (more concretely, I dunno how to do that well, esp. when having a pure Python base and meta-class that we may not explicitly control). (If y'all think I misread the solution, please let me know!)

I do believe I have solution to accommodate the intent of #2704, but while still maintaining the current setup for py::enum_. I'll be filing that shortly (read: within the next week).

Yes, "what about python2" is a good argument, but even pip is dropping support for python2 in January.

I'd be totes down for ditching Python 2! However, per the internal pybind11 Slack discussions, I'm unaware of explicit plans to cut Python 2 out in the future. I think the consensus was, "once CI becomes a P.I.T.A", which may not happen if we pin an older version of pip in that CI config?

Also, FWIW, I don't think it's a huge deal to conditionally support certain aspects in Python 2. I think it'd be easy to transparently make the Python 3 flavor behave like Enum, and possibly extend to the Flag flavors. I think, as the current py::enum_ implementation is defined, IntFlag will be implicitly possibly (i.e. ensuring closure of the types across bitwise operations).

AWhetter commented 3 years ago

I'm concerned about how much it deviates from the existing RTTI-based registration

For what I understand of the previous conversations on standard library enum support, this has always been the sticking point. I thought I would give it another attempt as a separate thing from enum, but it still raises weird API design questions because it's seemingly impossible to wrap standard library enums with an API exactly like class and enum_.

@wjakob has said previously that he would prefer to see enum rely on duck typing to act like standard library enums (https://github.com/pybind/pybind11/issues/253#issuecomment-228839251). I don't know if his opinion has changed, that is a fairly old comment after all, but I do think that it's going to give people want they want for 90% of use cases. It's probably the lowest friction way of improving the enum class and so long as we continue to keep them PEP 435 compatible, we still have the option of switching to true standard library enums in the future if someone can figure out how to do it.

EricCousineau-TRI commented 3 years ago

Thanks! Just to check:

It's probably the lowest friction way [...] we still have the option of switching to true standard library enums [...]

Can I ask what you mean by "it"? The macro + type_caster approach y'all have, or the meta-class approach that I'd like to try out? (I'm assuming the latter given the mention of duck typing, but just wanna make sure!)

FWIW Leveraging a Python base class may be simplified if we can simplify the instance registration system (e.g. decouple more complex stuff, so we could easily support this use case).

AWhetter commented 3 years ago

Can I ask what you mean by "it"?

I think that making the enum_ class look and act like an Enum is the lowest friction way.

bstaletic commented 3 years ago

Back when https://github.com/pybind/pybind11/pull/781 was still a thing, I did try to finish that pull request. You can guess that I got exactly nowhere. A bunch of times too. Even making py::enum34 arithmetic was surprisingly difficult. The idea at the time was "try it one way, but if a user calls def 'lock' the type into something that looks like py::class_". That allows users to call .def(), but the constructor of py::enum34 could not call def() too. One idea would be offloading to the users a need to call some finalization member function, but that still wouldn't solve something like this:

py::enum34<E>(m, "E").def("foo", foo).value("A", E::A).def("bar", bar)

Arguably, that's just stupid, but it's perfectly valid with py::enum_ today.

 

To be fair, I haven't looked too deeply into @AWhetter's PR. The above is just what I remember from a few years back.

anntzer commented 3 years ago

(*) actually last time I checked setattr'ing a cpp_function as a method doesn't actually work because they don't implement __get__-binding behavior, i.e. someobject.somemethod(arg) translates to somemethod(arg) instead of somemethod(someobject, arg) but I guess that's a separate issue...

(Edit: I guess using the C++-level destructor as "finalizer", as in #2704, would work...)

EricCousineau-TRI commented 3 years ago

I've filed an alternative draft implementation, #2744. I was wrong about __instancecheck__ and __subclasscheck__ (wrong direction :sweat_smile:). It's using some py::exec stuff, but I can rework it to direct C API :shrug:

This is one incantation of the duck-typed form that I believe Ashley mentioned above

I believe this won't be as good as Antony's end-result (in that it requires reimplementation), but it's a step in that direction, and not a huge diff. I also don't think reimplementing Flag will be too hard. We could end up doing something like that once we only support Python 3, and it's easy to figger out how to use other base classes*.

* On this point, I tried this out here, but got segfaults here: https://github.com/pybind/pybind11/pull/2744/commits/3bdf70c2f6de9c11cb15b2e072b1e62074db394d#diff-4249f25293fdbc32ba7cb0a011c27ff420af64a7f95931553f953dc41f38915eR1783-R1784 Tried both a direct subclass of the pybind11 base class, and something trying to wrap it.

auscompgeek commented 3 years ago

See also #1177 - pybind11 enums aren't singletons, but the enum module creates singletons per PEP 435.

anntzer commented 3 years ago

I'll allow myself to suggest once again having a way to bind stdlib enums (e.g. with some variant of my gist above, but I'm not wedded to the implementation), as it turns out that stdlib enums are still gaining new features (https://docs.python.org/3.10/library/enum.html#enum.FlagBoundary in py3.10 (the metaclass kwarg will require a small change to the gist, but shouldn't be too tricky either)) and it seems better to just reuse their implementation rather than reimplementing everything again and again on pybind11's side.

This can (should) even have a separate name (py::pystd_enum or whatnot) so that people can choose between the old and the new binding approaches...

Skylion007 commented 2 years ago

I am wondering if there is a cleaner way to fix this by using / abusing __new__ now that we have proper support for that.

BrukerJWD commented 2 years ago

I just want to note that with pybind11 2.10.0 the support for Python <3.5 (incl. 2.7) was removed, thus enum.Enum should be no problem from this perspective.