Skip to content

Conversation

@agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Jan 31, 2024

Description

GitHub Actions M1 runners are free for open-source repositories now – I have configured the PyPI workflow to add a job for these runners.

Important

The wheels can be tested on M-series hardware via the following link: https://github.com/pybamm-team/PyBaMM/actions/runs/7721068374/

Closes #3772, related to #3462

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Jan 31, 2024

Note to self:

  • Look into if archflags configuration is needed for CMake

Not needed because we are not cross-compiling at this time. CMake sets the variable CMAKE_CROSSCOMPILING to ON and cibuildwheel sets archflags as required.

  • Add CHANGELOG entry (af9c299)

  • Investigate whether to build universal2 wheel via delocate-fuse, delocate-addplat, or similar or to keep macOS wheels architecture-specific

We don't need dual-architecture wheels – pip prefers an architecture-specific binary distribution, which shall save both disk space and bandwidth for users shall we distribute it as such. The applications of universal2 binaries are more oriented towards .dmg or .pkg archives, or towards Python-based standalone applications such as those compiled by PyInstaller or Nuitka.

Python 3.8 wheels cannot be tested on arm64 devices but Python 3.9+ wheels can be. It would be a good idea to test all wheels across all Python versions.
@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review January 31, 2024 06:24
@agriyakhetarpal
Copy link
Member Author

We should start testing PyBaMM on M1 runners as well if we are going to be distributing them – I can do that in a follow-up PR, though I am not sure if #3140 (particle cracking submodels) has been resolved or not.

@codecov
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (063a383) 99.59% compared to head (310fbbf) 99.59%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3789   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          257      257           
  Lines        20802    20802           
========================================
  Hits         20718    20718           
  Misses          84       84           

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

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @agriyakhetarpal! This looks really good!

@Saransh-cpp
Copy link
Member

Also, all green ticks in this PR, nice!

@kratman
Copy link
Contributor

kratman commented Jan 31, 2024

I am not sure if #3140 (particle cracking submodels) has been resolved or not.

I put in a stop-gap fix for that ticket. I did not mark it as closed because all I did was adjust the grid. Not really a full fix or investigation. Having tests that would alert us sooner would probably make fixing stuff like that more visible and we could discuss closing that issue if we start testing more thoroughly on M-series hardware.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Jan 31, 2024

Having tests that would alert us sooner would probably make fixing stuff like that more visible and we could discuss closing that issue if we start testing more thoroughly on M-series hardware.

Sure, happy to merge this after you can download and test a wheel or two (I tested them, they are working for me since the IDAKLUSolver can be instantiated and can be used to solve a basic Simulation – even though that seems to not be a reliable test, see #3783). I have set up testing for M1 in #3791.

@kratman
Copy link
Contributor

kratman commented Jan 31, 2024

@agriyakhetarpal I downloaded the wheels to test them. I think they are working

pybamm.have_idaklu()
True

@kratman kratman merged commit df29f68 into develop Jan 31, 2024
@kratman kratman deleted the agriya-build-macos-silicon-wheels-new branch January 31, 2024 19:41
kratman added a commit that referenced this pull request Feb 1, 2024
…#3791)

* Add macOS M1 runner configuration for PR and scheduled tests

See #3789, #3462

* Add `macos-14` to test conditions

* Exclude Python 3.8 and 3.9 for now from testing

* Apply suggestions from code review

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>

* Remove some missed comments

---------

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
…ybamm-team#3789)

* Add configuration for macOS arm64 wheels

See pybamm-team#3772

* Build on Python 3.10+ for now

* Possibly incorrect version string parsing

* Missed adding link for `pybind11`

* pipx invocation is missing, installed by default on other runners

* Add user-facing CHANGELOG entry about M-series wheels

Python 3.8 wheels cannot be tested on arm64 devices but Python 3.9+ wheels can be. It would be a good idea to test all wheels across all Python versions.

* Add `always()` condition to ensure job will run
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
…pybamm-team#3791)

* Add macOS M1 runner configuration for PR and scheduled tests

See pybamm-team#3789, pybamm-team#3462

* Add `macos-14` to test conditions

* Exclude Python 3.8 and 3.9 for now from testing

* Apply suggestions from code review

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>

* Remove some missed comments

---------

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
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.

Use self-hosted macOS M2 runner to build macOS arm64 or universal wheels

4 participants