Skip to content
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

Add support for Flag enums #599

Closed
wants to merge 8 commits into from

Conversation

mwittgen
Copy link

With the major overhaul of enums in nanobind 2.0, the Flag enum type could be easily supported.
Some existing pybind11 applications rely on enums supporting bitwise operations with the result being an enum and not an int.

@mwittgen mwittgen force-pushed the support_flag_enums branch 4 times, most recently from 67664ab to 5a67564 Compare May 24, 2024 23:53
@mwittgen mwittgen marked this pull request as draft May 25, 2024 00:11
@chrreisinger
Copy link

Adding Flag support would be highly appreciated, since we use flags in our application and the new enum behaviour is blocking us from upgrading to 2.0.

@wjakob
Copy link
Owner

wjakob commented May 27, 2024

@chrreisinger

Adding Flag support would be highly appreciated, since we use flags in our application and the new enum behaviour is blocking us from upgrading to 2.0.

Could you tell me which behavior changed? The new enums should mostly behave like the old ones.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small first round of comments

@@ -1926,6 +1926,14 @@ The following annotations can be specified using the variable-length
mixed enum types (such as ``Shape.Circle + Color.Red``) are
permissible.

.. cpp:struct:: is_enum_flag
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flag_enum

operations. This enables the bitwise operators ``| & ^ ~``
with two enumeration as operands.
The result will an enumeration of the same type.
So ``Shape(2) | Shape(1) == Shepe(3)`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepe -> Shape

Previously, nanobind relied on a custom enumeration base class that was a
frequent source of friction for users.
A new flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to go to a separate changelog entry for 2.1.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you added the new changelog entry but didn't remove the old one.

docs/classes.rst Outdated
@@ -295,8 +295,12 @@ C++11-style strongly typed enumerations.

When the annotation :cpp:class:`nb::is_arithmetic() <is_arithmetic>` is
passed to :cpp:class:`nb::enum_\<T\> <enum_>`, the resulting Python type
will support arithmetic and bit-level operations like comparisons, and, or,
xor, negation, etc.
will support arithmetic and bit-level operations and, or,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and, or ..) (in parentheses)

/// Is the underlying enumeration type Flag?
is_flag_enum = (1 << 18)

// No more flag bits available (18). Needs
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we need this (as we undoubtedly will eventually), suggest splitting the type_init_flags to be in a separate field of type_init_data that is not preserved in type_data. That will free up six bits. I think some of the new type_flags might be able to move to type_init_flags also (is_generic?), which would free up further space.

@chrreisinger
Copy link

chrreisinger commented May 27, 2024

@chrreisinger

Adding Flag support would be highly appreciated, since we use flags in our application and the new enum behaviour is blocking us from upgrading to 2.0.

Could you tell me which behavior changed? The new enums should mostly behave like the old ones.

Sure.

A simplified version of the code is:

enum class Foo {
    A = 1,
    B = 2,
    C = 4,
    D = 8,
    E = 16,
    F = 32,
    ...
};
nb::enum_<pynf::Foo>(m, "Foo", nb::is_arithmetic())
        .value("A", pynf::Foo::A)
        .value("B", pynf::Foo::B)
        .value("C", pynf::Foo::C)
        .value("D", pynf::Foo::D)
        .value("E", pynf::Foo::E)
        .def("__bool__", [](const pynf::Foo val) { return pynf::to_underlying(val) != 0; })
        .def("__or__",
             [](const pynf::Foo a, const pynf::Foo b) {
                 return pynf::Foo(pynf::to_underlying(a) | pynf::to_underlying(b));
             })
        .def("__and__", [](const pynf::Foo a, const pynf::Foo b) {
            return pynf::Foo(pynf::to_underlying(a) & pynf::to_underlying(b));
        });

And at runtime we get an error that a combination of the flags is not valid.

| ValueError: 31095 is not a valid Foo.

@wjakob
Copy link
Owner

wjakob commented May 27, 2024

Aha! Yes, that makes sense. The fact that it worked previously is effectively undefined behavior. We expect that the user returns a valid/bound enum but do not check if that is actually the case.

@wjakob
Copy link
Owner

wjakob commented May 30, 2024

What's the status of this PR? Do you still plan to make changes? (it's in draft mode) Would you like me to review it?

@mwittgen
Copy link
Author

There are some more unresolved issues, for example
Flag.A | Flag.B can't be passed to a C++ function void func(enum_type e)
Looks like this needs some type casters for enum.Flag

@wjakob
Copy link
Owner

wjakob commented May 31, 2024

Yeah, that seems tricky. The existing type caster does a hash table lookup that looks for enumerants. In this case, you'd need to either need to populate the table with all possible combinations (combinatorial blowup) or perform some kind of integer conversion. I assume that the same issue would appear on the way back. The special case to support flag enums in this way might add a computational cost to the default enums (which I would not want). I don't have any suggestions, unfortunately.

@oremanj
Copy link
Contributor

oremanj commented May 31, 2024

It seems to me that it would work fine to fall back from the map lookups to a slightly slower alternate, either enum_instance.value for Py->C++ conversions or EnumClass(number) for C++->Py. This fallback could be done only for flag enums in order to avoid slowing down failing conversions of regular enums during overload resolution. Flag generates the objects representing flag combinations as they're requested, but they do still get "singletonized" / cached in the member2value_map, so we could choose to update our maps as well; then the slow path only needs to be taken once per combination. There are some other possible optimizations as well, such as caching the mask of which flags are defined, and preemptively rejecting lookups outside of that mask.

@mwittgen mwittgen force-pushed the support_flag_enums branch 2 times, most recently from 0f7d119 to 90a8ecb Compare June 2, 2024 16:43
@mwittgen mwittgen force-pushed the support_flag_enums branch from d496007 to 08dd9ef Compare June 18, 2024 01:06
@mwittgen mwittgen force-pushed the support_flag_enums branch from 08dd9ef to ab998d4 Compare June 18, 2024 01:07
@mwittgen
Copy link
Author

Fixed the 3.11+ compilation failures with stable ABI enabled.

@mwittgen mwittgen marked this pull request as ready for review June 18, 2024 01:23
@wjakob
Copy link
Owner

wjakob commented Jun 18, 2024

It would be helpful to have feedback from people who are invested into this -- @skallweitNV, @keithlostracco.

Does it look okay to you as well @oremanj? From what I can see, the modified PR now adopts your suggestion.

@skallweitNV
Copy link
Contributor

Question: Does it make sense to base nanobind's enum types on Enum and Flag in the first place? Wouldn't IntEnum and IntFlag (for enums with nb::is_arithmetic) be the more appropriate types? After all, we are binding C++ enums which are always of some integer type.

@wjakob
Copy link
Owner

wjakob commented Jun 18, 2024

For non-arithmetic enums (e.g. C++11-style enum classes), Enum is the closest match in Python-land. For arithmetic ones, we use IntEnum. That's how the implementation works now, and I think that part makes sense.

Is your point mainly about flags?

@keithlostracco
Copy link

This appears to be exactly the functionality I was looking for.

Copy link
Contributor

@oremanj oremanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach in general seems fine. I would like to see some more thought put into the performance of Python/C++ interconversions, and I have some documentation and style suggestions as well. (Ultimately it's Wenzel's opinion that matters here, not mine, so if he disagrees with something I suggested, his opinion wins.)

docs/api_core.rst Outdated Show resolved Hide resolved
Comment on lines 1933 to 1934
with two enumeration as operands.
The result will an enumeration of the same type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with two enumeration as operands.
The result will an enumeration of the same type.
with two enumerators as operands.
The result will have the same enumeration type as the operands.

The enumeration is the type; the named values are called enumerators. (It is a little bit ambiguous whether 3 is properly an "enumerator" if the defined values are 1 and 2, so I suggest wording so as to sidestep that question.)

Comment on lines 20 to 21
* The :cpp:class:`nb::enum_\<T\>() <enum_>` can now create an``enum.Flag``-derived type
with flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The :cpp:class:`nb::enum_\<T\>() <enum_>` can now create an``enum.Flag``-derived type
with flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>`.
* nanobind now allows a :cpp:class:`nb::enum_\<T\>() <enum_>` to specify the
new class binding annotation :cpp:class:`nb::is_flag_enum() <is_flag_enum>`,
which produces an enumeration type derived from `enum.Flag`. Instances of such
an enumeration type represent a bitwise combination of the defined enumerators,
and they may be combined using bitwise operators ``& | ^ ~``.

Previously, nanobind relied on a custom enumeration base class that was a
frequent source of friction for users.
A new flag :cpp:class:`nb::is_flag_enum() <is_flag_enum>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you added the new changelog entry but didn't remove the old one.

docs/classes.rst Outdated
will support arithmetic and bit-level operations like comparisons, and, or,
xor, negation, etc.
will support arithmetic and bit-level operations (and, or,
xor, negation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xor, negation).
xor, negation). The operands of these operations may be either enumerators
(including of two different `is_arithmetic` enumeration types) or integers, and the
result will be an integer.

src/nb_enum.cpp Outdated
if (is_flag_enum)
result.attr("__str__") = enum_mod.attr("Flag").attr("__str__");
#if PY_VERSION_HEX >= 0x030B0000
result.attr("_boundary_") = enum_mod.attr("FlagBoundary").attr("KEEP");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This choice seems not in keeping with nanobind's general defaults of strictness around enums. I think the default STRICT policy is more appropriate. People can use is_arithmetic (producing an IntFlag) if they want more flexibility. For IntFlag, KEEP is the default.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also my point above. We likely want the full matrix of combinations.

src/nb_enum.cpp Outdated
@@ -150,6 +154,28 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
if (!t)
return false;

if ((t->flags & (uint32_t) type_flags::is_flag_enum) !=0) {
auto base = PyObject_GetAttrString((PyObject *)o->ob_type, "__base__");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Python API calls return new object references which you must drop (Py_DECREF) when you're done using them. Also, this is way too much effort to spend on every individual enum-value conversion from Python to C++. You should do this only if the map lookup in enum_tbl.rev fails. If this slower-path conversion succeeds, then you can add to the enum_tbl.rev map to save time when converting the same flags value in the future.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually wondering if it makes sense to cache these. With flags, it would seem one can generate 2^n cache entries for n bits, which has the potential of being undesirably large. Maybe flags enums are OK to go on a slow path? Or we somehow restrict the size of the cache?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, they are already being cached in members of the enum type. This "singleton-izes" so that one can test for flag combinations with is.

>>> import enum
>>> class Test(enum.Flag):
...   A, B, C, D, E, F, G, H = 1, 2, 4, 8, 16, 32, 64, 128
...
>>> for i in range(256): Test(i)
...
<Test: 0>
<Test.A: 1>
<Test.B: 2>
[etc ...]
>>> len(Test._value2member_map_)
256

The existing cache takes, for each referenced combination, one dict entry (like 32 bytes I think?) plus the footprint of the enum value object itself (sys.getsizeof says 48 bytes, but that doesn't include the 4-entry instance dictionary, name string, etc -- it's probably at least 128 bytes total). We're adding one robin_map entry (24 bytes?) in each direction (C++->Py and Py->C++) for each combination that is passed across the language boundary. So the additional cost of caching is something like 30% above what we're already paying just for using enum.Flag. That seems reasonable to me.

src/nb_enum.cpp Outdated
auto basename = PyObject_GetAttrString(base, "__name__");
Py_ssize_t size;
const char* data = PyUnicode_AsUTF8AndSize(basename, &size);
std::string s(data, size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a sufficient reason to use std::string; nanobind generally tries to keep a low footprint of standard-library includes, to avoid code bloat. Check the size, then use memcmp().

Also, I don't think this logic is correct. You should instead check whether Py_TYPE(o) -- the Python type of the incoming might-be-an-enumeration-value -- matches t->type_py -- the Python type of the nanobind enumeration you're trying to convert to. What you have here is both much slower than that, and is foolable:

class Flag:
    pass

class Dummy(Flag):
    value = 42

some_nanobind_function(Dummy())  # result is treated as flags of 42!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/nb_enum.cpp Outdated
@@ -150,6 +154,28 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
if (!t)
return false;

if ((t->flags & (uint32_t) type_flags::is_flag_enum) !=0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the code style of the rest of this file. Spaces on both sides of binary operators, space after if (several instances of that further below), etc.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/nb_enum.cpp Outdated
enum_map *fwd = (enum_map *) t->enum_tbl.fwd;

if(t->flags & (uint32_t) type_flags::is_flag_enum) {
PyObject *value = PyLong_FromLongLong(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely wrong and the fact that it wasn't caught by a unit test means you need better test coverage. This will turn any flag-enum value into a plain Python int, i.e., it loses its flag-enum membership. That kind of defeats the point of a flag enum, and is extra confusing because you're not allowed to pass that thing back into a C++ function that expects an instance of the flag-enum type.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments

@@ -1926,6 +1926,14 @@ The following annotations can be specified using the variable-length
mixed enum types (such as ``Shape.Circle + Color.Red``) are
permissible.

.. cpp:struct:: is_flag_enum
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we give it a shorter name? is_flag? (for symmetry with the arithmetic flag, which is called is_arithmetic)

docs/api_core.rst Outdated Show resolved Hide resolved
@@ -138,9 +143,11 @@ according to `SemVer <http://semver.org>`__. The following changes are
noteworthy:

* The :cpp:class:`nb::enum_\<T\>() <enum_>` binding declaration is now a
wrapper that creates either a ``enum.Enum`` or ``enum.IntEnum``-derived type.
wrapper that creates either a ``enum.Enum``, ``enum.IntEnum`` or ``enum.Flag``-derived type.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to support a matrix of 4 base classes: nb::is_arithmetic switches between enum.* and enum.Int* while nb::is_flag switches between Enum and Flag.

src/nb_enum.cpp Outdated
@@ -1,5 +1,5 @@
#include "nb_internals.h"

#include <string>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid depending on std::string? I've tried hard not to have anything depend on it in the core library.

src/nb_enum.cpp Outdated
auto basename = PyObject_GetAttrString(base, "__name__");
Py_ssize_t size;
const char* data = PyUnicode_AsUTF8AndSize(basename, &size);
std::string s(data, size);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/nb_enum.cpp Outdated
@@ -150,6 +154,28 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
if (!t)
return false;

if ((t->flags & (uint32_t) type_flags::is_flag_enum) !=0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/nb_enum.cpp Outdated
if (is_flag_enum)
result.attr("__str__") = enum_mod.attr("Flag").attr("__str__");
#if PY_VERSION_HEX >= 0x030B0000
result.attr("_boundary_") = enum_mod.attr("FlagBoundary").attr("KEEP");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/nb_enum.cpp Outdated
if (is_flag_enum)
result.attr("__str__") = enum_mod.attr("Flag").attr("__str__");
#if PY_VERSION_HEX >= 0x030B0000
result.attr("_boundary_") = enum_mod.attr("FlagBoundary").attr("KEEP");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also my point above. We likely want the full matrix of combinations.

src/nb_enum.cpp Outdated
@@ -150,6 +154,28 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
if (!t)
return false;

if ((t->flags & (uint32_t) type_flags::is_flag_enum) !=0) {
auto base = PyObject_GetAttrString((PyObject *)o->ob_type, "__base__");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually wondering if it makes sense to cache these. With flags, it would seem one can generate 2^n cache entries for n bits, which has the potential of being undesirably large. Maybe flags enums are OK to go on a slow path? Or we somehow restrict the size of the cache?

@skallweitNV
Copy link
Contributor

For non-arithmetic enums (e.g. C++11-style enum classes), Enum is the closest match in Python-land. For arithmetic ones, we use IntEnum. That's how the implementation works now, and I think that part makes sense.

True, for class enums the Enum is the closest match.

Is your point mainly about flags?

Yes, I need support for flags. Supporting the full matrix for is_arithmetic and is_flag seems like the perfect solution.

Comment on lines 1933 to 1935
with two with two enumerators as operands.
The result will have the same enumeration type as the operands.
So ``Shape(2) | Shape(1)`` is equivalent to ``Shape(3)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with two with two enumerators as operands.
The result will have the same enumeration type as the operands.
So ``Shape(2) | Shape(1)`` is equivalent to ``Shape(3)`
with two enumerators as operands.
The result will have the same enumeration type as the operands.
So ``Shape(2) | Shape(1)`` is equivalent to ``Shape(3)`.

docs/classes.rst Outdated
Comment on lines 299 to 301
xor, negation). The operands of these operations may be either enumerators
When the annotation :cpp:class:`nb::flag_enum() <flag_enum>` is passed to
to :cpp:class:`nb::enum_\<T\> <enum_>`, the resulting Python type will be an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xor, negation). The operands of these operations may be either enumerators
When the annotation :cpp:class:`nb::flag_enum() <flag_enum>` is passed to
to :cpp:class:`nb::enum_\<T\> <enum_>`, the resulting Python type will be an
xor, negation). The operands of these operations may be either enumerators
When the annotation :cpp:class:`nb::flag_enum() <flag_enum>` is passed to
:cpp:class:`nb::enum_\<T\> <enum_>`, the resulting Python type will be an

Also, judging from the tests, the operands must be both enumerators?

src/nb_enum.cpp Outdated
@@ -56,7 +56,8 @@ PyObject *enum_create(enum_init_data *ed) noexcept {

if (is_arithmetic)
result.attr("__str__") = enum_mod.attr("Enum").attr("__str__");

if (is_flag)
result.attr("__str__") = enum_mod.attr("Flag").attr("__str__");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be IntFlag.__str__, since we use that factory above (L46)?

hawkinsp added a commit to hawkinsp/benchmark that referenced this pull request Jul 18, 2024
…bind 2.0.

Incorporates the nanobind_bazel change from google#1795.

nanobind 2.0 reworked the nanobind::enum_ class so it uses a real Python enum or intenum rather than its previous hand-rolled implementation.
google-deepmind/clrs#119 (comment)

As a consequence of that change, nanobind now checks when casting an integer to a enum value that the integer corresponds to a valid enum. Counter::Flags is a bitmask, and many combinations are not valid enum members.

This change:
a) sets nb::is_arithmetic(), which means Counter::Flags becomes an IntEnum that can be freely cast to an integer.
b) defines the | operator for flags to return an integer, not an enum, avoiding the error.
c) changes Counter's constructor to accept an int, not a Counter::Flags enum. Since Counter::Flags is an IntEnum now, it can be freely coerced to an int.

If wjakob/nanobind#599 is merged into nanobind, then we can perhaps use a flag enum here instead.
hawkinsp added a commit to hawkinsp/benchmark that referenced this pull request Jul 18, 2024
…bind 2.0.

Incorporates the nanobind_bazel change from google#1795.

nanobind 2.0 reworked the nanobind::enum_ class so it uses a real Python enum or intenum rather than its previous hand-rolled implementation.
https://nanobind.readthedocs.io/en/latest/changelog.html#version-2-0-0-may-23-2024

As a consequence of that change, nanobind now checks when casting an integer to a enum value that the integer corresponds to a valid enum. Counter::Flags is a bitmask, and many combinations are not valid enum members.

This change:
a) sets nb::is_arithmetic(), which means Counter::Flags becomes an IntEnum that can be freely cast to an integer.
b) defines the | operator for flags to return an integer, not an enum, avoiding the error.
c) changes Counter's constructor to accept an int, not a Counter::Flags enum. Since Counter::Flags is an IntEnum now, it can be freely coerced to an int.

If wjakob/nanobind#599 is merged into nanobind, then we can perhaps use a flag enum here instead.
hawkinsp added a commit to hawkinsp/benchmark that referenced this pull request Jul 18, 2024
…bind 2.0.

Incorporates the nanobind_bazel change from google#1795.

nanobind 2.0 reworked the nanobind::enum_ class so it uses a real Python enum or intenum rather than its previous hand-rolled implementation.
https://nanobind.readthedocs.io/en/latest/changelog.html#version-2-0-0-may-23-2024

As a consequence of that change, nanobind now checks when casting an integer to a enum value that the integer corresponds to a valid enum. Counter::Flags is a bitmask, and many combinations are not valid enum members.

This change:
a) sets nb::is_arithmetic(), which means Counter::Flags becomes an IntEnum that can be freely cast to an integer.
b) defines the | operator for flags to return an integer, not an enum, avoiding the error.
c) changes Counter's constructor to accept an int, not a Counter::Flags enum. Since Counter::Flags is an IntEnum now, it can be freely coerced to an int.

If wjakob/nanobind#599 is merged into nanobind, then we can perhaps use a flag enum here instead.
hawkinsp added a commit to hawkinsp/benchmark that referenced this pull request Jul 18, 2024
…bind 2.0.

Incorporates the nanobind_bazel change from google#1795.

nanobind 2.0 reworked the nanobind::enum_ class so it uses a real Python enum or intenum rather than its previous hand-rolled implementation.
https://nanobind.readthedocs.io/en/latest/changelog.html#version-2-0-0-may-23-2024

As a consequence of that change, nanobind now checks when casting an integer to a enum value that the integer corresponds to a valid enum. Counter::Flags is a bitmask, and many combinations are not valid enum members.

This change:
a) sets nb::is_arithmetic(), which means Counter::Flags becomes an IntEnum that can be freely cast to an integer.
b) defines the | operator for flags to return an integer, not an enum, avoiding the error.
c) changes Counter's constructor to accept an int, not a Counter::Flags enum. Since Counter::Flags is an IntEnum now, it can be freely coerced to an int.

If wjakob/nanobind#599 is merged into nanobind, then we can perhaps use a flag enum here instead.
dmah42 pushed a commit to google/benchmark that referenced this pull request Jul 18, 2024
…bind 2.0. (#1817)

Incorporates the nanobind_bazel change from #1795.

nanobind 2.0 reworked the nanobind::enum_ class so it uses a real Python enum or intenum rather than its previous hand-rolled implementation.
https://nanobind.readthedocs.io/en/latest/changelog.html#version-2-0-0-may-23-2024

As a consequence of that change, nanobind now checks when casting an integer to a enum value that the integer corresponds to a valid enum. Counter::Flags is a bitmask, and many combinations are not valid enum members.

This change:
a) sets nb::is_arithmetic(), which means Counter::Flags becomes an IntEnum that can be freely cast to an integer.
b) defines the | operator for flags to return an integer, not an enum, avoiding the error.
c) changes Counter's constructor to accept an int, not a Counter::Flags enum. Since Counter::Flags is an IntEnum now, it can be freely coerced to an int.

If wjakob/nanobind#599 is merged into nanobind, then we can perhaps use a flag enum here instead.
@wjakob
Copy link
Owner

wjakob commented Jul 24, 2024

What's the status of this PR? Is it ready to be merged? I see that there are still some failing tests related to stubs.

@@ -43,7 +43,7 @@ PyObject *enum_create(enum_init_data *ed) noexcept {
PyUnicode_FromFormat("%U.%U", scope_qualname.ptr(), name.ptr()));
}

const char *factory_name = is_arithmetic ? "IntEnum" : "Enum";
const char *factory_name = (is_arithmetic || is_flag) ? (is_flag ? "IntFlag" : "IntEnum") : "Enum";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not responsive to the previous discussion of which enum metaclass to use. is_flag without is_arithmetic should be Flag.

@oremanj
Copy link
Contributor

oremanj commented Jul 24, 2024

@wjakob I had not yet done another round of review because it seemed like there were review comments from the previous round that were unresolved - so I figured the author was still working on it. I flagged one thing I noticed.

@mwittgen
Copy link
Author

I was stuck on the python 3.10 failure. Will get back to this ticket.

@wjakob
Copy link
Owner

wjakob commented Aug 11, 2024

It seems to me that this is a feature that a number of people are waiting for. @mwittgen please let us know if you'd like someone else to continue developing the PR so that it can be merged.

@mwittgen
Copy link
Author

Yes, please.

@nicholasjng
Copy link
Contributor

I have some time in the upcoming days, and would like to advance this further.

@wjakob
Copy link
Owner

wjakob commented Aug 16, 2024

That sounds good. Could you create a new PR in this case @nicholasjng?

@wjakob
Copy link
Owner

wjakob commented Aug 21, 2024

Closing this PR, development continues in PR #688.

@wjakob wjakob closed this Aug 21, 2024
wjakob pushed a commit that referenced this pull request Sep 4, 2024
This PR extends nanobind with the capability to create flag enumerations (`enum.Flag`, `enum.IntFlag`) that support bit-wise arithmetic.

Co-authored-by: Matthias Wittgen <Matthias.Michael.Wittgen@cern.ch>
Co-authored-by: Nicholas Junge <nicholas.junge@web.de>
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 this pull request may close these issues.

7 participants