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

Support arm64 architecture on macOS #532

Merged
merged 10 commits into from
Jul 9, 2024
Merged

Support arm64 architecture on macOS #532

merged 10 commits into from
Jul 9, 2024

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented May 23, 2024

This PR responds to #531.

  • Improve .backend.jdbc:
    • Pass -Djava.library.path=… when starting the JVM, giving two directories containing the required binary libraries (with the extension .so on Linux; .dll on Windows; and .dylib on macOS):

      • The …/apifiles/Java/api/ directory within the user's GAMS installation directory.
      • The ixmp/backend/jdbc/ directory within the package itself, which contains (very old) bundled copies of the same files.

      The second directory is redundant if the first one is present (i.e. if a GAMS installation exists). To confirm this, the PR currently deletes the bundled libraries entirely. These commits could also be dropped instead.

  • Expand the CI matrix to include both x86_64 and arm64 architectures on the macOS.
    • Use an updated version of iiasa/actions/setup-gams per Handle macOS arm64 architecture in setup-gams actions#13. This version installs an architecture-appropriate GAMS, and handles caching automatically.
    • Exclude macos-latest (arm64) for Python 3.8 and 3.9, as there are no pre-built wheels for JPype1 for this combination.

How to review

  • Read the diff.
  • Note the CI checks all pass.
  • message_ix usage: Install ixmp from this branch on a macOS arm64 system with an arm64 version of Python. Run the test suite or other code and confirm the code functions as expected.
  • Scenario Explorer usage: Consider implications for non-message_ix deployments; particularly, for the Scenario Explorer. Is GAMS installed in these contexts? Will the absence of the .dylib, .so, and/or .dll files bundled with the ixmp Python package affect these deployments?

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update release notes.

@khaeru khaeru added the backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype label May 23, 2024
@khaeru khaeru self-assigned this May 23, 2024
Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.7%. Comparing base (f08447f) to head (97d9f30).

Current head 97d9f30 differs from pull request most recent head 1d40c22

Please upload reports for the commit 1d40c22 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #532     +/-   ##
=======================================
- Coverage   98.8%   98.7%   -0.1%     
=======================================
  Files         44      44             
  Lines       4804    4803      -1     
=======================================
- Hits        4747    4744      -3     
- Misses        57      59      +2     
Files Coverage Δ
ixmp/backend/jdbc.py 97.2% <100.0%> (-0.2%) ⬇️

... and 4 files with indirect coverage changes

@khaeru khaeru force-pushed the issue/531 branch 3 times, most recently from 97d9f30 to 1d40c22 Compare May 23, 2024 19:27
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.8%. Comparing base (cf56d03) to head (caaaa3d).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #532   +/-   ##
=====================================
  Coverage   98.8%   98.8%           
=====================================
  Files         44      44           
  Lines       4796    4813   +17     
=====================================
+ Hits        4739    4756   +17     
  Misses        57      57           
Files Coverage Δ
ixmp/backend/jdbc.py 97.4% <100.0%> (+<0.1%) ⬆️
ixmp/model/gams.py 96.2% <100.0%> (+0.3%) ⬆️

@khaeru khaeru force-pushed the issue/531 branch 4 times, most recently from 788ce9b to eb15a0e Compare May 24, 2024 14:23
@khaeru khaeru changed the title Add .dylib file for macOS/arm64 Support arm64 architecture on macOS May 24, 2024
@khaeru khaeru added the enh New features & functionality label May 24, 2024
@khaeru khaeru modified the milestone: 3.10 May 24, 2024
@khaeru khaeru marked this pull request as ready for review May 24, 2024 15:01
@khaeru
Copy link
Member Author

khaeru commented May 24, 2024

@glatterf42 let me know whether you think there is urgency to include this in the 3.9 release.

We would need to invite and wait for reviews from:

  • Ravit, Sidd, and/or Setu —to check behaviour on their machines.
  • Philip or someone else —to address the point about impacts on Scenario Explorer deployments.

Given this might take a few days, and the first three are probably capable to use it from main in the period between the 3.9 and 3.10 releases, I would lean to "no". OTOH, if we have users in the upcoming workshop who have macOS arm64 machines, this might (not necessarily) save some support overhead.

If we wait, then I will also wait until after the workshop to request a review of iiasa/actions#13.

@glatterf42
Copy link
Member

I'd also say it's likely not urgent. Workshop users will learn about GitHub anyway and if they need to use this branch, this will be a good opportunity to practice. They could also then provide feedback about their install process.
However, I think we can already invite review from our team members and if these come in very soon, we can maybe still include this in 3.9 :)

@khaeru
Copy link
Member Author

khaeru commented May 27, 2024

@ravitby @SiddharthJoshi-Git @setupelz —could you please try the steps above under "How to review" / "message_ix usage" and let us know the results?

@phackstock —could you please comment on the questions above under "How to review" / "Scenario Explorer usage"?

Copy link

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me @khaeru. As far as I know there's no GAMS installation for the Scenario Explorer.

Will the absence of the .dylib, .so, and/or .dll files bundled with the ixmp Python package affect these deployments?

I also don't think that that would be an issue.
Green light from my side to merge. If there are any issues that would arise from the Scenario Explorer side, we can always pin a previous version and open an issue here.

@khaeru
Copy link
Member Author

khaeru commented May 31, 2024

Great, thanks for confirming.

khaeru added a commit that referenced this pull request Jul 8, 2024
@khaeru khaeru force-pushed the issue/531 branch 2 times, most recently from 3678488 to 1176fd2 Compare July 8, 2024 09:50
@khaeru khaeru requested a review from glatterf42 July 8, 2024 10:44
@khaeru
Copy link
Member Author

khaeru commented Jul 8, 2024

@glatterf42, since the other requested colleagues seem unable to review this over the past ~6 weeks, I propose we merge and wait for feedback that may come as they/others use it from main. If we don't get positive confirmation before we start prep for the next release, then we can ask again.

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Sounds good to me, just one possible typo :)

RELEASE_NOTES.rst Outdated Show resolved Hide resolved
Include review suggestion by @glatterf42:
#532 (review)
- Use tempfile.TemporaryDirectory.
- Log warning if gams is missing.
- Create maximum one instance.
- Sort globals, classes, and functions in .model.gams
- Use macos-13 for Python 3.8–3.9.
- Use macos-latest for Python 3.10–3.12.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Work around libgmdjni64.dylib not compatible with arm64 architecture on macOS
4 participants