-
Notifications
You must be signed in to change notification settings - Fork 230
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
CI.yml
and environment.yml
updates to run CI on MacOS and Ubuntu
#2415
Conversation
Previous CI failed due to a limitation of macos runners, see byrnedo/docker-reg-tool#9. Updating now |
Switching to official Julia binaries, which may resolve the code signing problem, is blocked by the same libgfortran update that is impacting the upgrade to OpenMOPAC (i.e. potential fix is blocked by #2417). See failed CI run when attempting to use them. |
Potential bug in PATH setup for setup-julia action: julia-actions/setup-julia#89 |
Codecov Report
@@ Coverage Diff @@
## main #2415 +/- ##
==========================================
+ Coverage 48.11% 48.13% +0.02%
==========================================
Files 110 110
Lines 30626 30626
Branches 7988 7988
==========================================
+ Hits 14735 14743 +8
+ Misses 14362 14353 -9
- Partials 1529 1530 +1 see 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@calvinp0 could you review my updated installation instructions, which include warnings for apple silicon users? |
Merging this PR also:
|
- rmg::pydqed >=1.0.3 | ||
- rmg::pyjulia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is pyjulia installed through conda-forge::julia=1.8.5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not, and the rmg:pyjulia does not either. The conda-recipes repo has the build script and you can see that it only provides Julia (as far as I can tell, I'm new to building conda packages). I think pyjulia is an implicit dependency of something else in the environments file? So it is added when the environment is solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, correct if I am misunderstanding what you are saying, but we will need to keep pyjulia as it is the connection between Julia and Python. So I guess we can either keep getting from the rmg channel or from conda-forge channel.
However, the CI appears to be passing? I am a bit confused though how it is doing that when pyjulia isn't installed in the rmg_env (I am assuming here or really misunderstanding something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, correct if I am misunderstanding what you are saying, but we will need to keep pyjulia as it is the connection between Julia and Python. So I guess we can either keep getting from the rmg channel or from conda-forge channel.
However, the CI appears to be passing? I am a bit confused though how it is doing that when pyjulia isn't installed in the rmg_env (I am assuming here or really misunderstanding something)
My mistake - conda still ends up retrieving pyjulia from the rmg channel. See this line in the latest CI run.
CC'ing @mjohnson541 - pyjulia, how does it work? The conda-recipe doesn't make sense to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my understanding of PyJulia is that, assuming you have Julia installed and set in the PATH (or set in our conda env path for our case when rmg_env is active), it is imported in python like so:
import julia
Then we use PyJulia for the julia.install()
which does the whole PyCall adding (if not present) and building (unless it is M1, then it appears to have an issue with that
We can also use PyJulia to create our system images for precompiling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if that was already understood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a dependency of pyrms and diffeqpy which causes it to be installed as well.
conda create -n rmg_env | ||
conda activate rmg_env | ||
conda config --env --set subdir osx-64 | ||
conda install python=3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit pedantic and not really a concern or issue, but I figure we could remove the install of python=3.7 here as it will be install in the conda env update
part of the script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that seems like a wasted instruction. We should try the install from scratch on a mac without it just to be sure, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it last night on my Mac, so it installed fine without that command
CI.yml
and environment.yml
updates to run CI on MacOS and Ubuntu
I have updated |
Tagging a lot of people for review since this PR now contains (1) a different "required" check, which requires admin access and (2) changes to the environment that will change how RMG is run on MacOS. @hwpang and @rwest please provide general approval for this updated CI approach @calvinp0 please check the documentation changes and ensure they match the comments from the review @mjohnson541 please review to make sure that I did not describe any dependencies in the documented |
.github/workflows/CI.yml
Outdated
# | ||
# taken from https://github.com/orgs/community/discussions/4324#discussioncomment-3477871 | ||
ci-report-status: | ||
runs-on: ubuntu-20.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we specify the version ubuntu-20.04
here but ubuntu-latest
on line 257?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the place I copy-pasted from did it that way - should be latest instead so we don't have to update it as GitHub releases new and deprecates old runners. Thanks for catching this!
I will update now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also add a clarification for something that confused myself, this branch is called add-macos11-ci
but it actually runs on macos-latest
which as of writing (May 16, 2023) is MacOS 12 Monterey
@@ -84,6 +73,15 @@ Installation by Source Using Anaconda Environment for Unix-based Systems: Linux | |||
cd RMG-Py | |||
conda env create -f environment.yml | |||
|
|||
.. warning:: Apple Silicon (M1+) Users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same suggestion for Intel Mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it only applies to M1 users (cc @calvinp0 who figured this out, thanks!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions that could be separate pull requests (eg. trying to change channel for pyjulia) but some fixes to the user instructions that should probably be addressed before merging.
And a question about the benefit of adding a changelog to the environment file (which is already in git with - I hope - helpful commit messages).
@@ -128,6 +126,14 @@ Installation by Source Using Anaconda Environment for Unix-based Systems: Linux | |||
|
|||
julia -e 'using Pkg; Pkg.add(PackageSpec(name="ReactionMechanismSimulator",rev="main")); using ReactionMechanismSimulator;' | |||
|
|||
.. warning:: Apple Silicon (M1+) Users | |||
Execute the following commands instead of ``conda env create -f environment.yml``: :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't think you mean this "instead of...".
- one difference I notice with the M1 instructions is the "python import julia" comes before the "julia using Pkg" lines instead of after. Is this intentional and important? (if not maybe we can keep consistent)
- if the only difference is the M1 needs
julia -e 'using Pkg; Pk.add("PyCall");Pkg.build("PyCall")
maybe we could reduce the instructions to just say run this. (And is it really needed? and is there harm in everyone running it?) - if the instruction is to do something special before the default instructions, it would be good for the special instructions to come first. (so people don't do the default and then read "but first you should have..."). Like writing a cooking recipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# + some other categories (see below) | ||
# | ||
# Changelog: | ||
# - May 15, 2023 Added this changelog, added inline documentation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn about introducing a changelog, since one could always do git log -- environment.yml
. I could open to being persuaded though; when is a changelog in a file helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see changelogs is that they provide a "high-level" perspective on how the file has changed over time. The git log is useful for fixing technical issues and precise problems with specific lines, but I think the changelog is better for answering questions like "why do we have some packages from this channel but other from this one" or the like.
environment.yml
in particular has been going through substantial changes lately, and as we (hopefully) move toward using fewer and fewer in-house packages I think it will be useful to have this high level history of that activity. This is also a file that community developers will interact with a lot - such info would be particularly useful for them.
environment.yml
Outdated
- conda-forge::openbabel >= 3 | ||
|
||
# general-purpose external software tools | ||
- conda-forge::julia=1.8.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dare we try >=
and let the CI alert us if it breaks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put - conda-forge::pyjulia
here instead of the - rmg::pyjulia
down below? My impression from #2381 was that the answer is "yes" but maybe we can test it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on second thoughts, that could be a separate pull request if you prefer (and don't mind the churn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dare we try
>=
and let the CI alert us if it breaks?
Move fast and break things, I say. Will try now.
Can we put
- conda-forge::pyjulia
here instead of the- rmg::pyjulia
down below? My impression from #2381 was that the answer is "yes" but maybe we can test it here?
on second thoughts, that could be a separate pull request if you prefer (and don't mind the churn)
Every package we don't have to maintain is welcome - will try this as well.
if you're about to force push for new tests, maybe rebase onto main? (and squash some of the commits that undo each other?) |
|
IIRC this is done somewhere in the rmg channel Julia custom build
our build of Julia was setting these two environment variables automatically by putting them into the conda activate environment script. we should try to replicate that here, but for now I am hardcoding the result for the runner to see if it will fix the precompilation issues from the previous actions run
we cannot use branch protection rules on jobs that have matrix in them, so we add a summary step that will wait for both the Mac and Linux builds+tests to execute and then return pass or fail. we will need to update the branch protection rule for main to look for ci-report-status instead of -build-and-test-linux
e166e55
to
ff4b799
Compare
Sorry. The "Switch to conda-forge::pyjulia instead of the rmg channel" commit was meant to be on another branch (that's not working yet). Will drop it on rebasing when I get a chance. |
- break up the environment creation step s.t. the admonition for Mac M1 users are more clear - change the Julia and Python installation instructions so that they work for all platforms in the same order
This explanation was added in commit 5d20826 but I think it refers to the line ln -sfn $(which python-jl) $(which python) that was added in b6910a7 (2 minutes later) to the continuous integrations workflow, but not actually added to the user instructions. i.e. the linking step (that this explanation refers to) is not in the instructions. So I suggest we remove it.
Mostly whitespace and linebreaks. Added a new step number.
This changed many years ago now. Although some clusters (eg. at Northeastern) still have an old old anaconda, I no longer feel it's our job to explain how to use it, and just makes the instructions longer than needed.
i have added comments to the environment file, the issue is that our build of pyjulia does some `sed` magic that we do not replicate otherwise. would be good to get rid of this in the future, but this PR is already getting out of hand
- who needs pyjulia - why use conda-forge ncurses instead of default - gfortran is not listed in the env file, explain why
Otherwise the PyCall step installs it via conda! It looks like this: 279 dependencies successfully precompiled in 550 seconds. 14 already precompiled. [ Info: Installing rmgpy.molecule via the Conda rmg package... [ Info: Running `conda config --add channels rmg --file /opt/miniconda3/envs/rmg_env3/condarc-julia.yml --force` in root environment [ Info: Running `conda install -y rmg` in root environment (then you get the old binary from the rmg conda channel instead of your developer version) Or at least I *think* this is why that happened. When I did the whole thing again with this step included I didn't have the same problem.
The solver setting is environment-specific. If you set it before you make the rmg_env then it is not set IN the rmg_env and when you later do stuff to that environment, it's super slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Additional improvements can come in subsequent PRs.
👍 @rwest could you update the branch protection rules to look for |
@rwest that works as well - the |
This PR runs the Continuous Integration on a mac runner in addition to the Ubuntu runner, the idea being that we can catch issues that might crop up on the mac platform independently of debian linux. The results from the ubuntu runner are still considered to be the "baseline" for regression testing.
See #2355 and #2392 for issues related to running on Macs