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

Upgrade to Apple Silicon on CI, simplify Apple Silicon installation instructions #5973

Merged
merged 4 commits into from
Aug 27, 2024

Conversation

nilsvu
Copy link
Member

@nilsvu nilsvu commented May 8, 2024

Proposed changes

Upgrade the macOS CI test to run on Apple Silicon. Also simplify Apple Silicon installation instructions a bit.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@nilsvu nilsvu requested a review from nilsdeppe May 8, 2024 15:19
@nilsvu nilsvu force-pushed the installation_instructions branch from d01d48b to 039aeba Compare May 8, 2024 15:34
nilsdeppe
nilsdeppe previously approved these changes May 8, 2024
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

Thank you!

@knelli2
Copy link
Contributor

knelli2 commented May 8, 2024

MacOS spack installation of pybind11 isn't happy

@nilsvu nilsvu force-pushed the installation_instructions branch 2 times, most recently from 7129b4b to a39c58d Compare May 8, 2024 21:00
Comment on lines 978 to 980
-D BUILD_PYTHON_BINDINGS=ON \
-D BOOTSTRAP_PY_DEPS=ON \
Copy link
Contributor

Choose a reason for hiding this comment

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

The SetupPybind11.cmake isn't finding the boostrapped pybind11. Maybe we need another hint in cmake?

@nilsvu nilsvu force-pushed the installation_instructions branch 9 times, most recently from 4d4d782 to c9d9218 Compare May 12, 2024 18:03
@nilsvu nilsvu mentioned this pull request May 18, 2024
3 tasks
@nilsvu nilsvu force-pushed the installation_instructions branch 2 times, most recently from 2b186a7 to 2f4fecf Compare June 3, 2024 19:34
@nilsvu nilsvu changed the title Remove libsharp & brigand from installation instructions, container Simplify Apple Silicon installation instructions, upgrade to Apple Silicon on CI Jun 3, 2024
@nilsvu nilsvu force-pushed the installation_instructions branch 3 times, most recently from d925c24 to f07dcf3 Compare June 3, 2024 20:09
@nilsvu
Copy link
Member Author

nilsvu commented Jun 3, 2024

@geoffrey4444 FYI. Still needs #5956 to be merged first.

@nilsvu nilsvu force-pushed the installation_instructions branch 2 times, most recently from a324de0 to 6362c38 Compare June 3, 2024 20:19
@geoffrey4444
Copy link
Contributor

I'm excited about adding Apple Silicon to the CI...thanks for doing this!

I understand there are complexities to #5956 , so if that looks like it won't get merged for a while, maybe consider just implementing the slightly longer instructions in a separate PR, so we can start automatically checking Apple Silicon? We can always improve things later, but we have enough Apple Silicon users that it would be nice to get the CI up and running. But if $5956 is going to get merged soon-ish, maybe a separate PR isn't worth it.

For those who prefer manual installation (I'm probably one of them, at least for now), maybe instead of deleting the longer instructions, move them down to a "manually install dependencies" section?

@nilsvu nilsvu force-pushed the installation_instructions branch from b6e5388 to 428d3ea Compare June 5, 2024 05:14
@nilsvu
Copy link
Member Author

nilsvu commented Jun 5, 2024

@geoffrey4444 ok I removed the dependence on #5956 for now. Once it's merged we can simplify the instructions further, but I agree that it would be good to keep an (optional) "install manually" step.

@nilsvu nilsvu mentioned this pull request Jun 21, 2024
3 tasks
@nilsvu nilsvu force-pushed the installation_instructions branch 3 times, most recently from 8b6cb08 to e806e81 Compare June 21, 2024 21:48
@nilsvu nilsvu force-pushed the installation_instructions branch 2 times, most recently from 17a28c6 to 6fdfe6f Compare August 20, 2024 21:18
@nilsvu
Copy link
Member Author

nilsvu commented Aug 20, 2024

@nilsdeppe @geoffrey4444 @knelli2 I rebased and updated this. Please, could whoever of you is interested in this take another look so we can get this in? It would be helpful to have an ARM macOS build on CI.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Changes look ok to me, but I don't have a mac to test.

Comment on lines 163 to +164
-D CHARM_ROOT=${SPECTRE_DEPS_ROOT}/charm/multicore-darwin-arm8 \
-D SPECTRE_FETCH_MISSING_DEPS=ON \
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's up to @geoffrey4444 if he wants the docs default to install extra dependencies in people's build directories

Copy link
Contributor

Choose a reason for hiding this comment

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

I think fetching missing dependencies is fine

@nilsvu nilsvu force-pushed the installation_instructions branch 2 times, most recently from cc56424 to e7fa645 Compare August 21, 2024 22:18
nilsdeppe
nilsdeppe previously approved these changes Aug 22, 2024
@geoffrey4444
Copy link
Contributor

geoffrey4444 commented Aug 23, 2024

LGTM, but I notice it has conflicts that must be resolved first

@nilsvu
Copy link
Member Author

nilsvu commented Aug 24, 2024

Rebased!

knelli2
knelli2 previously approved these changes Aug 24, 2024
@knelli2
Copy link
Contributor

knelli2 commented Aug 26, 2024

Tolerances on the partial derivative test still seem to not be adequate.

@nilsdeppe
Copy link
Member

No need for my approval on this. I was pinged in the past, but have nothing to add.

This test failed on Intel macOS with OpenBLAS.
@nilsvu
Copy link
Member Author

nilsvu commented Aug 27, 2024

Tolerances seem to work now @knelli2

@knelli2 knelli2 merged commit d5db196 into sxs-collaboration:develop Aug 27, 2024
23 checks passed
@nilsvu nilsvu deleted the installation_instructions branch August 27, 2024 23:45
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.

4 participants