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

Bump nanobind to stable v1.4.0 tag #1626

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

nicholasjng
Copy link
Contributor

This seems to reduce binding sizes even further, with a wheel size of 175KB on my local machine (macOS 13.4.1).

@@ -31,6 +31,7 @@ cc_library(
"src/nb_internals.cpp",
"src/nb_internals.h",
"src/nb_ndarray.cpp",
"src/nb_static_property.cpp",
Copy link
Member

Choose a reason for hiding this comment

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

should this be sourced as an external library from somewhere (github release tarball?) so we don't need to have a parallel build of the library maintained here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be used as a build-time CMake dependency via scikit-build, but this requires changing the package build to use scikit-build - that could be an avenue.

Otherwise, there is an open JAX PR that ports some existing pybind11 code to nanobind - there they have a leaner build file, but still explicit configs (I think that is sensible too tbh)

In the end, however, we are pulling straight from github in Bazel too, so this might already be what you intended? (The build file is necessary to mark it as a Bazel dep)

Copy link
Member

Choose a reason for hiding this comment

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

i thought similar to how we depend on googletest, we could depend on nanobind. but maybe it's a different type of dependency if we don't directly link to it? or maybe nanobind doesn't support bazel directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, nanobind only supports CMake. So I'm afraid we need the full build file setup regardless. But at least the link above shows that it can still be simplified, especially if we don't particularly care about pinning the submodules.

This seems to reduce binding sizes even further, with a wheel size of
175KB on my local machine (macOS 13.4.1).
@dmah42 dmah42 merged commit cb39b71 into google:main Jul 11, 2023
56 of 60 checks passed
@nicholasjng nicholasjng deleted the bump-nanobind-to-14 branch April 15, 2024 16:46
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.

2 participants