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

build: Use build-system pybind11 over Git submodule #313

Merged

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Sep 12, 2024

Resolves #309

  • Update lower bound on build-system pybind11 to v2.12.0 to add support for compiling for NumPy 2.0.
  • Remove pybind11 submodule to use build-system instead.
  • Add an easy to parse log of the environment used for each test, which is useful for debugging differences between CI jobs seperated in time.

For reference, the pybind11 Git submodule was added in 0f9f8ff by @jpivarski, though as this wasn't part of a PR I'm not sure why the Git submodule approach was used over the other pybind11 install methods.

@henryiii
Copy link
Member

henryiii commented Sep 12, 2024

Why do you have pybind11 and numpy as build requirements? Pybind11 doesn't require numpy at build time. That's one of it's key selling points, actually.

@henryiii
Copy link
Member

I don't see any usage of numpy.h.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 12, 2024

Why do you have pybind11 and numpy as build requirements?

Error on my part as I didn't do a git grep to check and just followed Issue #309:

... and to also compile against numpy 2 when building.

I'll fix this now. Thanks for pointing this out.

@matthewfeickert matthewfeickert force-pushed the build/make-numpy-2-compatible branch from d8e069d to 9cbe78c Compare September 12, 2024 21:05
@matthewfeickert
Copy link
Member Author

I don't see any usage of numpy.h.
...
Why do you have pybind11 and numpy as build requirements? Pybind11 doesn't require numpy at build time. That's one of it's key selling points, actually.

Ah yes, as you point out

#include <pybind11/numpy.h>

pybind11 takes care of that for us.

@@ -2,7 +2,7 @@
requires = [
"setuptools>=42",
"setuptools_scm[toml]>=3.4",
"pybind11>=2.6.1",
"pybind11>=2.12.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this isn't really necessary(?) to bump it given that there's already a lower bound here, but I guess signaling this also doesn't hurt.

@matthewfeickert matthewfeickert changed the title build: Add support for NumPy 2.0 wheels build: Ensure support for NumPy 2.0 wheels Sep 12, 2024
@matthewfeickert
Copy link
Member Author

@lgray can you comment on the original issues that caused you to open Issue #309? I'm not actually clear on what about fastjet should be incompatible with NumPy 2.0.

@matthewfeickert matthewfeickert changed the title build: Ensure support for NumPy 2.0 wheels build: Use build-system pybind11 over Git submodule Sep 12, 2024
@matthewfeickert matthewfeickert force-pushed the build/make-numpy-2-compatible branch from 9cbe78c to e6eb88f Compare September 12, 2024 23:56
@matthewfeickert matthewfeickert force-pushed the build/make-numpy-2-compatible branch from e6eb88f to 0d9041c Compare September 13, 2024 03:52
* Update lower bound on build-system pybind11 to v2.12.0 to add support for
  compiling for NumPy 2.0.
   - c.f. https://github.com/pybind/pybind11/releases/tag/v2.12.0
* Remove pybind11 submodule to use build-system instead.

fixup
* Add an easy to parse log of the environment used for each test, which is useful
  for debugging differences between CI jobs seperated in time.
@matthewfeickert
Copy link
Member Author

As no one has commented yet, I'm going to merge this myself. As always, PRs approved by a single dev can be reverted as needed by the rest of the dev team.

@matthewfeickert matthewfeickert merged commit 1a4f68c into scikit-hep:main Sep 16, 2024
13 checks passed
@matthewfeickert matthewfeickert deleted the build/make-numpy-2-compatible branch September 16, 2024 23:42
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.

fastjet is not numpy 2 compatible
2 participants