Skip to content

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

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

Open
frank-xlj opened this issue Jul 27, 2020 · 21 comments · May be fixed by #2744
Assignees
Milestone

Comments

@frank-xlj
Copy link

frank-xlj commented Jul 27, 2020

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

got it. Thanks.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Jul 27, 2020

Seems solved/answered?

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Dec 4, 2020

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 EricCousineau-TRI self-assigned this Dec 4, 2020
@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Dec 4, 2020

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
Copy link
Contributor

anntzer commented Dec 8, 2020

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
Copy link
Collaborator

EricCousineau-TRI commented Dec 23, 2020

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 🙄

@EricCousineau-TRI
Copy link
Collaborator

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

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Dec 23, 2020

Moving comment from #2739 (comment) (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
Copy link
Contributor

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 (#253 (comment)). 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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

Back when #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
Copy link
Contributor

anntzer commented Dec 24, 2020

  • My implementation relies on macros because it was not written with integration into pybind11 itself in my mind. I guess introducing the right C++ base class could make a similar approach work fine without macros.
  • I believe my approach basically already works for Py27 -- it's just incumbent on the caller to replace "enum.Enum" by whatever backport base class they want to use, e.g. "enum34.Enum" (of course they'd now have a dependency on that, but note that they can also choose to just vendor enum34 as myproject.enum and bind to "myproject.enum.Enum"). Personally I find the ability to bind to enum.Flag quite important, and it seems a bit silly to have to fully reimplement an equivalent class in pybind11...
  • Python stdlib enums are fundamentally predicated on having all values defined at once, so I guess the pybind11 approach of using .value() for definition won't work (well, you can make the users call a finalizer but ugh). OTOH I think you could have require all values to be defined in a single call, e.g.
py::stdenum("PyName", "enum.Enum")
  .values({"name", value}, {"name", value}, ...)  // or without the braces
  .def("method", method_impl)
  .def("method", method_impl)

as you can always add methods later to the Python class (you can just setattr them... (*)). (It's up to you whether the C base enum class needs to be a template argument, as you can normally just infer it from the values passed to .values().

(*) 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
Copy link
Collaborator

EricCousineau-TRI commented Dec 24, 2020

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

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:
3bdf70c#diff-4249f25293fdbc32ba7cb0a011c27ff420af64a7f95931553f953dc41f38915eR1783-R1784
Tried both a direct subclass of the pybind11 base class, and something trying to wrap it.

@auscompgeek
Copy link
Contributor

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

@anntzer
Copy link
Contributor

anntzer commented Jun 2, 2021

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
Copy link
Collaborator

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

@Skylion007 Skylion007 added this to the v3.0 milestone Oct 31, 2021
@BrukerJWD
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants