Switch bindings implementation to nanobind
#1526
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmah42
reviewed
Jan 12, 2023
Removes Python 3.7 from the support matrix, since it does not support PEP590 vectorcalls. Bumps the `cibuildwheel` and `pypa-publish` actions to their latest available versions respectively.
The build file builds nanobind as a static `cc_library`. Currently, the git SHA points to HEAD, since some necessary features have not been included in a release yet.
Switches over the binding tool to `nanobind` from `pybind11`. Most changes in the build setup itself were drop-in replacements of existing code changed to nanobind names, no new concepts needed to be implemented. Sets the minimum required macOS to 10.14 for full C++17 support. Also, to avoid ambiguities in Bazel, build for macOS 11 on Mac ARM64.
Guards against unknown linker option errors by selecting required linker options for nanobind only on macOS, where they are relevant. Other changes: * Bump cibuildwheel action to v2.12.0 * Bump Bazel for aarch64 linux wheels to 6.0.0 * Remove C++17 flag from build files since it is present in setup.py `bazel build` command * Bump nanobind commit to current HEAD (TBD: Bump to next stable release)
nicholasjng
force-pushed
the
with-nanobind
branch
from
a46120b to
d3c940c
Compare
dmah42
previously approved these changes
Jan 30, 2023
|
Hmm, I guess I need to pass the |
Guards compiler options behind a new `select` macro choosing between MSVC and not MSVC. Other changes: * Inject the proper C++17 standard cxxopt in the `setup.py` build command. * Bump nanobind to current HEAD. * Make `macos` a benchmark-wide condition, with public visibility to allow its use in the nanobind BUILD file.
nicholasjng
force-pushed
the
with-nanobind
branch
from
52c0187 to
b6b795f
Compare
Since `benchmark::Counter` only has a constructor for `double`, the nanobind `nb::init_implicit` template cannot be used. Therefore, to support implicit construction from ints, we fall back to the `nb::implicitly_convertible` template instead.
nicholasjng
force-pushed
the
with-nanobind
branch
from
b6b795f to
61a925d
Compare
|
I fixed the MSVC breakage, which was due to (my) user error. Should be good to go now, binding test matrix is all green, other failures are due a downed apt mirror presumably. If you want, I can restructure the PR and split it up (e.g. carving out the CI parts), to make it easier to revert to |
dmah42
approved these changes
Feb 6, 2023
|
thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Title is self-explanatory. This uses
nanobindto build Python bindings for Google Benchmark instead ofpybind11, with the following consequences:nanobindis checked in as a bazel dep, currently at HEAD instead of a tag, because some features used to generate the GBM binding code have not made it into a release yet.For performance considerations and changes in contrast to the current implementation, see the discussion #1521.
(For reviewers: I am not sure of the options I gave in the nanobind BUILD file - I looked at what compile commands CMake generated in release mode for nanobind and went on from there.)
The bindings now also contain the state name from #1511.