feat(PyTreeKind): use pybind11::native_enum to create enum class PyTreeKind
#214
Conversation
XuehaiPan
added
enhancement
XuehaiPan
changed the title
PyTreeKindpybind11::native_enum to create enum class PyTreeKind
XuehaiPan
force-pushed
the
native-enum
branch
2 times, most recently
from
93f1b6d to
a8faff8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 1302 1302
Branches 155 155
=========================================
Hits 1302 1302 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for native enum types in the PyTreeKind enum by leveraging the new pybind11::native_enum feature when available, along with updated version checks for Python and pybind11.
- Introduces conditional compilation blocks for native enum support.
- Updates immutability handling for PyTreeKind, PyTreeSpec, and PyTreeIter types.
- Enforces newer version requirements for Python and pybind11 in the macros header.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/optree.cpp | Adds native enum support for PyTreeKind and updates immutability. |
| include/optree/pymacros.h | Updates version checks and rearranges namespace declaration. |
Comments suppressed due to low confidence (1)
src/optree.cpp:28
- [nitpick] Consider adding an inline comment to clarify the rationale behind the combined condition for enabling native enum support to aid future maintainability.
#if defined(PYBIND11_HAS_NATIVE_ENUM) || (defined(PYBIND11_INTERNALS_VERSION) && PYBIND11_INTERNALS_VERSION >= 8)
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new feature to define the PyTreeKind enum class using pybind11::native_enum when available, falling back to the existing py::enum_ approach otherwise. Key changes include:
- Adding preprocessor checks and macro definitions in src/optree.cpp to detect native enum support.
- Updating dependency references in pyproject.toml and the lint workflow to pull the latest pybind11 version.
- Enforcing minimum version checks in include/optree/pymacros.h.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/optree.cpp | Adds conditional support for pybind11::native_enum and adjusts type flags. |
| pyproject.toml | Updates pybind11 dependency to use the latest GitHub source. |
| include/optree/pymacros.h | Adjusts preprocessor version checks for Python and pybind11. |
| .github/workflows/lint.yml | Installs the updated pybind11 dependency in CI. |
XuehaiPan
force-pushed
the
native-enum
branch
2 times, most recently
from
018d358 to
70798e7
Compare
XuehaiPan
force-pushed
the
native-enum
branch
2 times, most recently
from
0092f05 to
7765488
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new feature that conditionally creates the enum class PyTreeKind using pybind11::native_enum when available, and refines related module initialization and build steps. Key changes include:
- Adding conditional compilation logic in src/optree.cpp to support pybind11::native_enum.
- Updating header version checks in include/optree/pymacros.h.
- Enhancing CI workflows to allow testing with nightly pybind11 builds.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/optree.cpp | Uses pybind11::native_enum conditionally and replaces PyType_Ready calls with PyType_Modified. |
| include/optree/pymacros.h | Adjusts version checks and organizes namespace declarations. |
| CHANGELOG.md | Updates changelog with entries for the new enum and build improvements. |
| .github/workflows/tests.yml | Adds step for testing with nightly pybind11 builds. |
| .github/workflows/tests-with-pydebug.yml | Adds step for testing with nightly pybind11 builds. |
| .github/workflows/set_setup_requires.py | Provides a script to update pyproject.toml to use nightly pybind11. |
| .github/workflows/lint.yml | Adds nightly pybind11 installation step during linting. |
| .github/workflows/build.yml | Revises conditions for triggering build jobs in workflows. |
Comments suppressed due to low confidence (2)
src/optree.cpp:29
- [nitpick] Consider adding a brief comment explaining the rationale behind the conditional inclusion of native_enum to assist future maintainers.
#if defined(PYBIND11_HAS_NATIVE_ENUM) || (defined(PYBIND11_INTERNALS_VERSION) && PYBIND11_INTERNALS_VERSION >= 8)
.github/workflows/set_setup_requires.py:19
- [nitpick] Review the regex pattern to ensure it accurately targets the pybind11 dependency declaration without over-matching; consider refining it for clarity if necessary.
r'(requires\s*=\s*\[.*"\s*)pybind11[^"']*(\s*".*\])'
f968884 to
041a79c
Compare
XuehaiPan
force-pushed
the
native-enum
branch
4 times, most recently
from
bd749c0 to
044842e
Compare
Description
Describe your changes in detail.
Motivation and Context
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax
close #15213if this solves the issue #15213Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!
make format. (required)make lint. (required)make testpass. (required)