-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
Add GitHub Actions for CI #855
Conversation
Add GitHub CI
Codecov Report
@@ Coverage Diff @@
## develop #855 +/- ##
========================================
Coverage 97.55% 97.55%
========================================
Files 233 233
Lines 12083 12083
========================================
Hits 11788 11788
Misses 295 295
Continue to review full report at Codecov.
|
Sounds like very compelling arguments! I'm happy to switch to this if you can get it set up. The only thing is I'd still like to keep test CI time as short as possible even if we can go over 50 minutes (but CRON jobs can be longer) |
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've been trying to understand why the sundials aren't found on macos.
On macos, setting GitHub Actions environment variables, for instance with
env:
LD_LIBRARY_PATH: 'SuiteSparse-5.6.0/lib:sundials/lib'
DYLD_LIBRARY_PATH: 'SuiteSparse-5.6.0/lib:sundials/lib'
does not actually set the corresponding shell variable. It does on Ubuntu.
Example:
# Global env variable
env:
LD_LIBRARY_PATH: ${{ env.HOME }}/.local/lib
- name: Print variable
run: |
echo "#########"
echo $LD_LIBRARY_PATH
echo ${{ env.LD_LIBRARY_PATH }}
echo "#########"
On a macos runner, this yields:
1 Run echo "#########"
2 echo "#########"
4 echo $LD_LIBRARY_PATH
3 echo /Users/runner/.local/lib
4 echo "#########"
5 shell: /bin/bash -e {0}
6 env:
7 LD_LIBRARY_PATH: /Users/runner/.local/lib
8 #########
9
10 /Users/runner/.local/lib
11 #########
On an Ubuntu runner, the shell variable LD_LIBRARY_PATH
is correctly set:
1 Run echo "#########"
2 echo "#########"
4 echo $LD_LIBRARY_PATH
3 echo /home/runner/.local/lib
4 echo "#########"
5 shell: /bin/bash -e {0}
6 env:
7 LD_LIBRARY_PATH: /home/runner/.local/lib
8 #########
9 /home/runner/.local/lib
10 /home/runner/.local/lib
11 #########
I have no experience with MacOS, so don't know if I'm missing something obvious here.. or if it's a problem from GitHub Actions?
Therefore a working solution is to manually export LD_LIBRARY_PATH
before running the tests:
run: |
export LD_LIBRARY_PATH=SuiteSparse-5.6.0/lib:sundials/lib
python run-tests.py --unit --folder all
This works on both platforms.
.github/workflows/test_on_push.yml
Outdated
|
||
env: | ||
LD_LIBRARY_PATH: 'SuiteSparse-5.6.0/lib:sundials/lib' | ||
DYLD_LIBRARY_PATH: 'SuiteSparse-5.6.0/lib:sundials/lib' |
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 thats required as LD_LIBRARY_PATH
has precedence over DYLD_LIBRARY_PATH
.
See the apple docs
.github/workflows/test_on_push.yml
Outdated
- name: Install Linux system dependencies | ||
if: matrix.os == 'ubuntu-latest' | ||
run: | | ||
sudo apt install gfortran gcc libopenblas-dev liblapack-dev graphviz libsuitesparse-dev |
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.
libsuitesparse-dev
is actually not required as SuiteSparse is installed from sources later
.github/workflows/test_on_push.yml
Outdated
mkdir -p third-party/pybind11 | ||
git clone https://github.com/pybind/pybind11.git third-party/pybind11 | ||
python setup.py install_all -f | ||
source ~/.bashrc |
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.
If I understand correctly, each command in GitHub steps is ran using /bin/bash -e {0}
, which won't source the .bashrc
(I think?) Furthermore each run:
entry is executed in a separate shell, thus not inheriting previous env variables
As far as I can tell, this version of the workflow should be able to execute the tests in the three platforms, including those related to Sundials in MacOS, which were failing before. I've tried to use the new installation scripts for Many thanks to @tlestang for pointing me in the right direction! |
.github/workflows/test_on_push.yml
Outdated
run: | | ||
pip install wget | ||
python scripts/setup_KLU_module_build.py | ||
pybamm_install_odes --install-sundials |
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.
pybamm_install_odes
is an entry point defined in the setup.py, so I'm surprised the command is found before pybamm is installed!
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.
setup_KLU_module_build.py
already installs the sundials in $HOME/.local/
, so the pybamm_install_odes
command could be replaced by
export SUNDIALS_INST= ${{ HOME }}/.local
pip install scikits.odes
.github/workflows/test_on_push.yml
Outdated
run: | | ||
brew update | ||
brew install graphviz | ||
brew install openblas suitesparse |
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.
suitesparse isnt required since compiled and installed later on
@dalonsoa I agree that it can be confusing :) The
Does that make sense? |
All good, now! |
Description
Implements GitHub Actions as the CI framework. The rationale for this suggestion is the following:
I've open this PR as a draft because, at the moment, it does not fully cover the features of the existing testing platform - although it does a more extensive testing in other aspects. In particular:
Fixes # None
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.
Key checklist:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: