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

Run pre-commit autoupdate, fix stale pyproject.toml comments #1712

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

nicholasjng
Copy link
Contributor

black was replaced by ruff-format, so the comments were stale.

@nicholasjng
Copy link
Contributor Author

oioioi, looks like the bindings are incompatible with bzlmod.

@nicholasjng
Copy link
Contributor Author

@macandy13 I saw that you pushed nanobind v1.2.0 into the Bazel registry, do I need to push other tags in order to be able to use them?

Until the newer nanobind tags are pushed to the BCR, it's best to disable
bzlmod for the bindings, because the Python CI breaks due to Bazel 7
enabling bzlmod by default.
@dmah42 dmah42 merged commit c2de526 into google:main Dec 13, 2023
80 checks passed
@dmah42
Copy link
Member

dmah42 commented Dec 13, 2023

thanks!

@nicholasjng nicholasjng deleted the dev-updates branch December 13, 2023 14:27
@macandy13
Copy link
Contributor

Sorry for the late reply.

Building the Python bindings with bzlmod was waiting on some changes in rules_python, but I lost track of the work there as I switched projects in the meantime. IIRC the problem I faced back then was that the BUILD targets for the bindings depend on the currently installed Python version, and it was very hard to get the C++ headers of that installed Python interpreter to build in bzlmod.

So I don't think it is easy to solve this problem with just using nanobind from the bzlmod registry. My feeling back in June was that setup.py should be replaced by some Python version specific bazel targets, but that was quite a drastic change and was not compatible with the CI approach taken here, so I kept my hands off it.

Seems like it is time to revisit this. Happy to brainstorm what could be good paths forward now that bzlmod is the de-facto standard.

@nicholasjng
Copy link
Contributor Author

Thanks for getting back to me!

I saw a related discussion by someone trying to push nanobind to vcpkg, and the answer there really resonated with me: wjakob/nanobind#341 (comment)

In short, nanobind is so simple and small, yet so configurable that it is recommended to bootstrap everywhere instead of pulling from a centralized registry such as vcpkg or in this case the BCR.

I agree very much with that assessment - build flags provided by the guy pushing the nanobind tags to the BCR might not work for everyone, and some users might want or need to configure things differently (e.g., we currently do not have stable ABI support here in GBM yet).

The second part of why I think this is the way to go is the Python version. I think rules_python is the way better approach than just sourcing the current system Python's headers like we do now, but registering a toolchain for every version might be overkill.

Then again, rules_python devs seem to make it a point to follow strict hermeticity. I'm not deep enough into the Bazel Python ecosystem, so all tips are appreciated.

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.

3 participants