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

Solve issues with Python wheel building #546

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

janden
Copy link
Collaborator

@janden janden commented Sep 4, 2024

This PR does a few things:

  • Make cmake output a verbose makefile that tells us what commands are actually run.
  • Move some of the CIBW config settings for Windows into pyproject.toml.
  • Force wheels to be built with -march=x86_64 to enhance compatibility (this seems to be an issue in particular running wheels on macOS with x86_64 compatibility mode on ARM chips). This excludes the macOS arm64 wheels, of course.

This commit does a few things:
* Make cmake output a verbose makefile that tells us what commands are
  actually run.
* Move some of the CIBW config settings for Windows into pyproject.toml.
* Force wheels to be built with `-march=x86_64` to enhance compatibility
  (this seems to be an issue in particular running wheels on macOS with
  x86_64 compatibility mode on ARM chips). This excludes the macOS arm64
  wheels, of course.
@janden janden marked this pull request as ready for review September 4, 2024 16:40
@janden janden requested a review from lu1and10 September 4, 2024 16:40
[[tool.cibuildwheel.overrides]]
select = "*arm64*"
# Don't enforce x84_64 when building on arm64.
config-settings = {"cmake.define.FINUFFT_ARCH_FLAGS" = "", "cmake.define.CMAKE_VERBOSE_MAKEFILE" = "ON"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we can still use march=native no? AFAIK from M1 to M3 they all support NEON only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to see this tested on those machines before enabling it. Maybe for 2.4?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I compiled locally on my machine (apple-m1) with:
cmake ../ -DFINUFFT_ARCH_FLAGS=-mcpu=apple-m3 -DFINUFFT_USE_OPENMP=OFF -DFINUFFT_BUILD_TESTS=ON -DCMAKE_C_COMPILER=gcc-14 -DCMAKE_CXX_COMPILER=g++-14

and it passes the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok will add that, then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this address a fix for 2.3.0, ie should it come in now and go in the release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The macOS x86_64 wheels were still crashing under Rosetta 2 (emulation mode for ARM chips), so we had to fix some things.

Should be supported by all M* CPUs.
@ahbarnett ahbarnett added this to the 2.3 milestone Sep 5, 2024
@ahbarnett
Copy link
Collaborator

since this is a fix to 2.3.0, I'm bringing in.

@ahbarnett ahbarnett merged commit ed79798 into flatironinstitute:master Sep 5, 2024
167 checks passed
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