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.

Sign up for GitHub

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

Jump to bottom

Add support for macOS arm64 wheels #1649

Merged
merged 7 commits into from
Feb 28, 2022
Merged

Add support for macOS arm64 wheels #1649

merged 7 commits into from
Feb 28, 2022

Conversation

alexsavulescu
Copy link
Member

@alexsavulescu alexsavulescu commented Feb 21, 2022 •
edited
Loading

  • update build_wheels.bash logic for osx wheels
  • add build_static_readline_osx.bash
  • fix neurondemo for arm64
  • update docs

@alexsavulescu @pramodk
Add support for macOS arm64 wheels
116680a 
* update build_wheels.bash logic for osx wheels
* add build_static_readline_osx.bash
* fix neurondemo for arm64
* update docs

Co-authored-by: Pramod Kumbhar <pramod.s.kumbhar@gmail.com>
@alexsavulescu alexsavulescu linked an issue Feb 21, 2022 that may be closed by this pull request
Support for manually building macOS arm64 wheels #1647
Closed
@alexsavulescu
Copy link
Member Author

Have done sanity checks (import neuron; neuron.test();neuron.test_rxd()). Here are the wheels (had to zip them in order to be able to upload them here in the GH comment)

NEURON_nightly-8.0a747-cp310-cp310-macosx_11_0_arm64.whl.zip
NEURON_nightly-8.0a749-cp39-cp39-macosx_11_0_arm64.whl.zip
NEURON_nightly-8.0a749-cp38-cp38-macosx_11_0_arm64.whl.zip

nrnhines
nrnhines reviewed Feb 22, 2022
View reviewed changes
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

There is a lot here I don't understand. Can't really do a substantive review. Perhaps I should finish off my last question about MACOS_DEPLOYMENT_TARGET for #1570 . It is not clear to me why this has to be limited to arm64.

docs/install/install_instructions.md Show resolved Hide resolved
docs/install/python_wheels.md Outdated Show resolved Hide resolved
@alexsavulescu
Copy link
Member Author

It is not clear to me why this has to be limited to arm64.

We were focusing on getting wheels support for arm64 in time for the paper. Ideally we would have only universal2 for python 3.9 and 3.10 (and perhaps 3.8?) but we reckon that would be much more time invested to have something working. I guess it's better to start off with arm64 separate wheels and work our way from there towards universal2.

tristan0x
tristan0x reviewed Feb 22, 2022
View reviewed changes
packaging/python/build_static_readline_osx.bash Outdated Show resolved Hide resolved
packaging/python/fix_demo_libnrnmech.sh
Comment on lines -12 to +16
libnrnmech_dir=$instdir/share/nrn/demo/release/x86_64
if [[ `uname -m` == 'arm64' ]]; then
libnrnmech_dir=$instdir/share/nrn/demo/release/arm64
else
libnrnmech_dir=$instdir/share/nrn/demo/release/x86_64
fi
Copy link
Member

Choose a reason for hiding this comment

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

libnrnmech_dir=$instdir/share/nrn/demo/release/`uname -m`

?

Copy link
Member Author

Choose a reason for hiding this comment

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

heh, tempting! but for legacy reasons I'd rather not ..

Copy link
Member

Choose a reason for hiding this comment

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

In the context of Arm64 I am thinking about @tristan0x's suggestion because on circleci-arm we now have aarch64 😒. Looking at CMake docs:

image

So this definitely needs more care!

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh, I'd like to see the platform disappear entirely.

Copy link
Member

Choose a reason for hiding this comment

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

I completely agree, but that would be for next (major) release. But as we started doing fixes for Apple M1 and AArch64, we need to such changes anyway. (I will PR circle-ci soon)

@alexsavulescu alexsavulescu marked this pull request as draft February 22, 2022 12:35
@alexsavulescu
* address reviews
eff2752 
* always export MACOSX_DEPLOYMENT_TARGET=10.9
@alexsavulescu alexsavulescu marked this pull request as ready for review February 23, 2022 15:20
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022 •
edited by codecov bot
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.20%. Comparing base (9a7ff90) to head (3256818).
Report is 1864 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1649      +/-   ##
==========================================
+ Coverage   45.16%   45.20%   +0.03%     
==========================================
  Files         551      551              
  Lines      111189   111189              
==========================================
+ Hits        50217    50261      +44     
+ Misses      60972    60928      -44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@neuronsimulator neuronsimulator deleted a comment from nrnhines Feb 25, 2022
@neuronsimulator neuronsimulator deleted a comment from nrnhines Feb 25, 2022
@alexsavulescu alexsavulescu mentioned this pull request Feb 25, 2022
Closed
@pramodk pramodk modified the milestones: Frontiers paper, Release v8.1 Feb 28, 2022
pramodk
pramodk approved these changes Feb 28, 2022
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM

@pramodk pramodk merged commit e5a0303 into master Feb 28, 2022
@pramodk pramodk deleted the arm64_osx_wheels branch February 28, 2022 14:30
@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
Closed
15 tasks
@olupton olupton mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrnhines nrnhines nrnhines approved these changes

@pramodk pramodk pramodk approved these changes

@tristan0x tristan0x tristan0x left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release v8.1
Development

Successfully merging this pull request may close these issues.

Support for manually building macOS arm64 wheels
5 participants
@alexsavulescu @codecov-commenter @tristan0x @pramodk @nrnhines