-
Notifications
You must be signed in to change notification settings - Fork 192
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
Enable CI on Github #3571
Enable CI on Github #3571
Conversation
2f588e1
to
dee017d
Compare
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.
Thanks a lot @sphuber for all the work, this is great!
Since there are quite a few new files, could you perhaps describe in a few words what has changed?
That would make a detailed review a bit quicker.
@ltalirz just added an overview, let me know if something is still missing. |
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.
Overall looks good to me, some minor suggestions
3eb6d19
to
e90914d
Compare
@ltalirz I think the current state then is ready. I will then add a commit to remove Travis and simplify the remaining code necessary to run on Jenkins |
1aa77e3
to
cdef5de
Compare
This is essentially ready. Once we have released
|
42b2d77
to
00fb5d7
Compare
Add a GitHub workflow `.github/workflows/ci.yml` that will trigger on push and pull requests. It will run the five actions it defines: * `conda`: will install conda and create the environment * `docs`: builds the documentation with nitpick warning * `pre-commit`: runs pre-commit on all files * `tests`: runs `verdi devel tests` and the stand alone test files * `verdi`: runs the tests that check the load time is acceptable All tests are performed for python 3.7 except for the `tests` action that is done for 3.5 as well. Both python versions are run for both database backends in a matrix strategy. Even though we support 3.6 as well we do not explicitly test it since it will require another two builds and testing 3.5 and 3.7 should give decent guarantees. Finally the argument for multiple individual actions instead of joining them is based on the fact that there does not seem to be a limit on concurrent number of actions on GitHub as of this writing. This means that by spreading them out, allows running them in parallel which should reduce the overal runtime of the continuous integration workflow.
00fb5d7
to
64c63ad
Compare
@ltalirz with |
GitHub workflow run can be found here:https://github.com/sphuber/aiida_core/commit/64c63ad75dd3821c02ed2ad4b813140f7bad68e8/checks?check_suite_id=338130505 |
The majority of tests are now run through a GitHub CI workflow. This means it is not necessary to also run those on Jenkins, especially that Jenkins was originally configured to just run additional tests that were more computationally intensive. Therefore, we only keep the RPN test to run on Jenkins for now.
64c63ad
to
fab1c56
Compare
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.
thanks @sphuber , I've gone through everything and looks good to go!
run: | ||
.github/workflows/tests.sh | ||
|
||
verdi: |
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.
still seems like a bit of a waste to do all of the installation just to run one test...
anyhow, it's fine for 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.
Let's bask in the superfluous wealth of practically unlimited parallel builds :)
Fixes #3553
I did not touch the files for the Jenkins tests yet. I want to see what it is exactly that we want to run there and if possible simplify the scripts and remove what we no longer need. The current setup also does not include coverage yet.
To move the CI to GitHub actions, I created a workflow
.github/workflows/ci.yml
.This defines 5 different actions:
conda
: will run.github/workflows/conda.sh
to install conda and create the environment fromenvironment.yml
docs
: will build the documentationpre-commit
: runs pre-commit on all the filestests
: runs.github/workflows/tests.sh
to run all tests for both backends, for now only for python 3.7 but other versions can easily be added in the matrix. The tests include the entire unit test suite throughverdi devel tests
as well as the various stand alone tests in.ci
verdi
: tests the loading speed of verdiThe reason I put the actions in separate actions is that many of them do not require the same setup and like this they will run in parallel making the total run time shorter. I also feel that the files are clearer this way and it is easier to understand what is going on and what actions is responsible for what. The only downside of this approach is if we would run out of actions slots, but it seems there is only a limit on number of concurrent workflows, which at 20 per repository I don't think we will hit very often.
Finally, to simplify the configuration of profiles, computer and codes for the
tests
action, I have created.yaml
configuration files in.github/config
which are called in.github/workflows/setup.sh
.