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

Removing the macos-12 runner #4520

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Oct 16, 2024

Description

The macos-12 runner is being deprecated in December. This switches to the macos-13 runner

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

@kratman kratman marked this pull request as ready for review October 16, 2024 19:50
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (fb81f21) to head (717dd7c).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4520   +/-   ##
========================================
  Coverage    99.42%   99.42%           
========================================
  Files          299      299           
  Lines        22715    22715           
========================================
  Hits         22584    22584           
  Misses         131      131           

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

@kratman kratman marked this pull request as draft October 16, 2024 22:17
@kratman
Copy link
Contributor Author

kratman commented Oct 16, 2024

Converting to a draft while I take a look at the failure. It does appear to be repeatable

@agriyakhetarpal
Copy link
Member

It looks like it's coming from the MLIR stuff, since Python 3.11 on macOS is the only Python version where it's enabled

@kratman
Copy link
Contributor Author

kratman commented Oct 16, 2024

It looks like it's coming from the MLIR stuff, since Python 3.11 on macOS is the only Python version where it's enabled

Yeah it looks like it would not have been attempted in the old one with macos-12:

            if (not sys.version_info[:2] == (3, 11)) or mac_ver < 13:
                warnings.warn(
                    (
                        "IREE is only supported on MacOS 13 (or higher) and Python"
                        "version 3.11. Setting PYBAMM_IDAKLU_EXPR_IREE=OFF."
                    ),
                    stacklevel=2,
                )
                return "OFF"

@kratman
Copy link
Contributor Author

kratman commented Oct 17, 2024

@agriyakhetarpal The version of IREE used before was a nightly build not a stable build, so I will try using the stable one instead

@@ -40,7 +40,7 @@ def set_iree_state():
# iree-compiler is currently only available as a wheel on macOS 13 (or
# higher) and Python version 3.11
mac_ver = int(platform.mac_ver()[0].split(".")[0])
if (not sys.version_info[:2] == (3, 11)) or mac_ver < 13:
if (not sys.version_info[:2] == (3, 11)) or mac_ver < 14:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary fix to make this work. I will create an issue for it

@kratman
Copy link
Contributor Author

kratman commented Oct 17, 2024

@agriyakhetarpal Let me know if this temporary fix is acceptable. I created this issue for solving the MacOS-13 issue #4521

@agriyakhetarpal
Copy link
Member

Yes, I think we can't do much about this, so this is acceptable for me

@kratman kratman marked this pull request as ready for review October 17, 2024 10:43
@kratman kratman requested a review from a team as a code owner October 17, 2024 10:43
Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Ah, I didn't realise my review was dismissed. I'm personally not a fan of this setting (we experimented with it and later removed it in the pybamm-cookie repo), since the "Require conversation resolution before merging" option worked well.

@kratman
Copy link
Contributor Author

kratman commented Oct 17, 2024

Ah, I didn't realise my review was dismissed. I'm personally not a fan of this setting (we experimented with it and later removed it in the pybamm-cookie repo), since the "Require conversation resolution before merging" option worked well.

Except I have seen a bunch of PRs get merged with real changes that never got reviewed due to the lack of having this setting. It is a necessary setting for a good project

@agriyakhetarpal
Copy link
Member

Sure, let's keep this in that case, then!

@agriyakhetarpal agriyakhetarpal merged commit 82f7a0f into pybamm-team:develop Oct 17, 2024
26 checks passed
@agriyakhetarpal agriyakhetarpal deleted the feat/updateMacRunner branch October 17, 2024 13:20
pkalbhor pushed a commit to pkalbhor/PyBaMM that referenced this pull request Nov 15, 2024
* Removing the macos-12 runner

* Update to latest IREE stable release

* Try a different stable release

* Change jax version

* last attempt before reverting jax-iree changes

* Hack to make keep this running
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.

2 participants