Use GitHub Actions for CI #92

ml-evs · 2019-11-22T12:57:20Z

This PR was initially created to add linting to our repo separately to travis, and it has now evolved into the following changes, authored jointly with @CasperWA:

On all PRs, and pushes to master, the following will run:

flake8:
- currently this is required to pass before merging, but the only checks that break this are syntax errors and undefined names. Other errors (e.g. line too long, of which we have many due to our docstrings...) appear as warnings that do not prevent merging.
pre-commit (which only runs black at the moment)
- this currently never blocks merges, but perhaps should.
install this package with the all requirements pinned and run the tests, under Python 3.7 and 3.8.
- obviously blocks merging.
- codecov upload occurs on the 3.7 build only.
- currently backend requirements have been left floating in the hope that they do not change their API frequently.
install the package with floating requirements, under both 3.7 and 3.8 and run tests again.
- this never blocks merges, but allows us to keep an eye on things as they break and make separate dependency updates when needed.
- backend requirements are pinned to major versions due to expected breakages.

codecov · 2019-11-22T12:58:50Z

Codecov Report

Merging #92 into master will decrease coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   85.36%   85.02%   -0.34%     
==========================================
  Files          45       44       -1     
  Lines        2425     2371      -54     
==========================================
- Hits         2070     2016      -54     
  Misses        355      355

Flag	Coverage Δ
#unittests	`85.02% <100%> (?)`

Impacted Files	Coverage Δ
optimade/models/utils.py	`87.5% <100%> (ø)`	⬆️
optimade/filtertransformers/elasticsearch.py	`87.74% <100%> (ø)`	⬆️
optimade/models/optimade_json.py	`84% <0%> (-2.21%)`	⬇️
optimade/models/structures.py	`81.3% <0%> (-2.04%)`	⬇️
optimade/models/links.py	`83.33% <0%> (-1.29%)`	⬇️
optimade/validator/validator.py	`60.73% <0%> (-1.05%)`	⬇️
optimade/filtertransformers/mongo.py	`74.19% <0%> (-1.01%)`	⬇️
optimade/filtertransformers/tests/test_django.py	`88.23% <0%> (-0.66%)`	⬇️
optimade/models/jsonapi.py	`90.36% <0%> (-0.55%)`	⬇️
optimade/models/baseinfo.py	`92.3% <0%> (-0.29%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6675838...786cd5e. Read the comment docs.

ml-evs · 2019-12-01T18:56:14Z

Hopefully this can now be merged after #95 and #99 (after bumping the versions in requirements.txt and setup.py), following the dicussion in #96

CasperWA · 2019-12-04T11:20:29Z

I have played around with this on my own implementation, and have set it up for black. Since we are using black for linting, do you not also want to use that here, instead of flake8?

CasperWA · 2019-12-04T11:44:53Z

Ah, I seem to have lost the point of re-introducing requirements.txt. I will revert my change to delete it. Sorry.

ml-evs · 2019-12-04T11:51:32Z

I have played around with this on my own implementation, and have set it up for black. Since we are using black for linting, do you not also want to use that here, instead of flake8?

They perform different functions no? I'm happy with running the pre-commit here too, but a blackened repo isn't necessarily flake8 compliant. We could also add e.g. pylint on top of flake8 to detect further bad code smells.

Re-introduce requirements.txt with static versions for base dependencies. Create three different dependency GitHub Actions jobs: - static - eager - as user

CasperWA · 2019-12-04T11:55:53Z

I have played around with this on my own implementation, and have set it up for black. Since we are using black for linting, do you not also want to use that here, instead of flake8?

They perform different functions no? I'm happy with running the pre-commit here too, but a blackened repo isn't necessarily flake8 compliant. We could also add e.g. pylint on top of flake8 to detect further bad code smells.

Ah, didn't know this. Yeah sure. I'll add it in again.

ml-evs · 2019-12-04T12:05:38Z

This looks good, but now we've veered into replacing travis entirely (fine). Do we know how to prevent a particular step from blocking a merge (e.g. the future deps)? If we want to go all the way with this, codecov can be added to the gh action too.

CasperWA · 2019-12-04T13:20:01Z

This looks good, but now we've veered into replacing travis entirely (fine).

True. But maybe that's fine? We just need to know how to move releasing over as well...

Do we know how to prevent a particular step from blocking a merge (e.g. the future deps)? If we want to go all the way with this, codecov can be added to the gh action too.

codecov can indeed be added, using codecov-action.
I am not sure how to block merging for a single test. Maybe it's fine just to block merge if any of the tests fail, since we also want "pretty"/linted code?

ml-evs · 2019-12-05T12:41:37Z

Do we maybe only want to run Actions on push if it is on the master branch, and then for PRs always?

The danger here being that actions will not be run on WIP branches until a PR is made.
But we also "only" have 2k minutes of Actions run-time on this account.

Oh, that's interesting, I assumed it would be unlimited use (of course, until they have everyone using it...) In that case your suggestion sounds sensible!

CasperWA · 2019-12-05T12:58:30Z

But we also "only" have 2k minutes of Actions run-time on this account.

Oh, that's interesting, I assumed it would be unlimited use (of course, until they have everyone using it...)

Nope. See here.
Since Materials-Consortia is a Github Team for Open Source, we only get 2k minutes, whereas a GitHub Team would get 10k.

ltalirz · 2019-12-05T13:06:43Z

The 2k limit is for private repos

CasperWA · 2019-12-05T13:23:43Z

The 2k limit is for private repos

My mistake - glitch-reading the page. Thanks for the eye-opener.

ml-evs · 2019-12-05T19:16:12Z

I've updated the description of this PR, so please "review" that too, but I think this is now ready to go. We still need to update the installation and contributing instructions, I'll add that to the other PR over at #68.

CasperWA · 2019-12-06T00:18:04Z

pre-commit (which only runs black at the moment)

this currently never blocks merges, but perhaps should.

I agree. In particular, I suggest we have a develop branch (after we create a proper release) and have pre-commit be blocking for merging to master but not necessarily to develop?

install this package with the pinned core requirements and run the tests, under Python 3.7 and 3.8.

obviously blocks merging.

codecov upload occurs on the 3.7 build only.

currently backend requirements have been left floating in the hope that they do not change their API frequently. I would favor pinning to at least major versions of these packages.

After some thought, I agree. Especially since I saw that django just release v3 the other day. Having this stay at v2 for this test absolutely makes sense. For the one mentioned below, however, it is essential that we install the very latest.

install the package with floating requirements, under both 3.7 and 3.8 and run tests again.

this never blocks merges, but allows us to keep an eye on things as they break and make separate dependency updates when needed.

CasperWA · 2019-12-06T00:19:55Z

Btw, I will add the "publish on PyPI" workflow in another PR. I think that's better? However that also means that between this as that, the repository looses the ability to auto-publish on valid tag pushes.

CasperWA

Happy with this.

In order to accommodate our latest wishes, requirements.txt needs to be updated with django>=2.* or django~=2 or something similar. I am not exactly sure what is correct to make sure all minor version are updated, but the major is not.
If we update the setup.py with restrictive versions, the eager test will not run properly, I think.

.github/workflows/deps_lint.yml

ml-evs · 2019-12-06T13:05:59Z

In order to accommodate our latest wishes, requirements.txt needs to be updated with django>=2.* or django~=2 or something similar. I am not exactly sure what is correct to make sure all minor version are updated, but the major is not.

I think maybe django>=2,<3 will work?

If we update the setup.py with restrictive versions, the eager test will not run properly, I think.

Yes, we end up back in the scenario of multiple requirements files... Since the backends only require only one package each, we could hardcode the pip installation of the latest version in the eager tests?

CasperWA · 2019-12-06T13:13:31Z

In order to accommodate our latest wishes, requirements.txt needs to be updated with django>=2.* or django~=2 or something similar. I am not exactly sure what is correct to make sure all minor version are updated, but the major is not.

I think maybe django>=2,<3 will work?

I have looked more into this. We can use ~=. The way I understand it is that pip will update to the newest version of the last version specified, i.e.:
django~=2.2 will install the latest minor version, never exceeding major version 2.
django~=2.2.1 will install the latest patch version for major.minor version 2.2.

This notation is quite minimalist, which I like, but at the same time, it may not be very informative to the uninitiated.

So I would probably update setup.py with these, i.e., just replacing the first = with a ~.

If we update the setup.py with restrictive versions, the eager test will not run properly, I think.

Yes, we end up back in the scenario of multiple requirements files... Since the backends only require only one package each, we could hardcode the pip installation of the latest version in the eager tests?

Let's just not do my suggestion here? And hardcoding fails to meet the automation aspect of the workflow job, I think?

ml-evs · 2019-12-06T13:15:11Z

Ah sorry, misunderstood your comment. I thought by restrictive versioning you were referring to your own suggestion. I'll do this now.

CasperWA · 2019-12-06T13:16:37Z

Ah sorry, misunderstood your comment. I thought by restrictive versioning you were referring to your own suggestion. I'll do this now.

Ah, yeah. I kind of was. But I was thinking of adding these things to requirements.txt or setup.py.
If we only ever use requirements.txt for testing, I think it's fine to add all dependencies here. Indeed, it would actually make the most sense for testing.

- Restricted versions of backends to major versions in setup.py - Added pinned versions of backends to requirements.txt - Moved requirements.txt to be CI specific

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

CasperWA · 2019-12-06T15:16:21Z

I tried to be clever. I started to write a script to update the requirements files if setup.py was updated. But in the end, it was too much work for a very small gain.
So now there are just manually maintained requirements files ...

CasperWA · 2019-12-06T15:21:07Z

Another question I have pondered: Should we truly be eager in the upgrade and remove all constraints, or should we keep to what the workflow is set up for now, i.e. keep the constraint for pydantic, and introduce similar constraints for the eager requirements in the future if we need it?

I guess this comes down to, what is the real purpose of the eager test job?

ml-evs · 2019-12-07T17:13:12Z

Another question I have pondered: Should we truly be eager in the upgrade and remove all constraints, or should we keep to what the workflow is set up for now, i.e. keep the constraint for pydantic, and introduce similar constraints for the eager requirements in the future if we need it?

I guess this comes down to, what is the real purpose of the eager test job?

Honestly, I would remove pydantic as an eager requirement altogether, so we instead are testing whether we support the latest version that FastAPI also supports (currently FastAPI has pydantic>=0.32,<2.0.0 in their reqs).

I'll make this change now, then I really think we should merge this as it is going to hold up other PRs (the checks in this PR are now required but cannot run on the others!)

CasperWA

Great! And I agree about pydantic and the purpose of the eager test.

Thanks a lot @ml-evs for starting and doing this PR. It was quite helpful for myself to learn Actions :)

ml-evs added 2 commits November 22, 2019 12:56

Try out Github actions for linting

7acf0ca

Fix python version and rename

0f364f3

ml-evs and others added 3 commits November 22, 2019 15:51

Merge branch 'master' into gh_action_linter

a612b0d

Add noqa tags to conlist hack

85d00b7

Merge branch 'master' into gh_action_linter

6f97509

ml-evs mentioned this pull request Nov 28, 2019

Use services for testing/updating dependencies? #96

Closed

ml-evs and others added 2 commits November 28, 2019 12:02

Merge branch 'master' into gh_action_linter

6a2e265

Re-pinned basic dependencies, but test latests versions with actions

545d797

ml-evs force-pushed the gh_action_linter branch from 89051f5 to 545d797 Compare November 28, 2019 12:28

ml-evs added 3 commits November 28, 2019 12:32

Removed extraneous hyphens

32b36e2

Added pytest install

fb06eee

Bump dep testing to [all]

046271e

ml-evs requested review from ltalirz and CasperWA December 1, 2019 18:55

Merge branch 'master' into gh_action_linter

87884d1

lint -> pre-commit. Remove requirements.txt

1292317

CasperWA force-pushed the gh_action_linter branch from 53d3ce2 to 1292317 Compare December 4, 2019 11:31

CasperWA added 2 commits December 4, 2019 12:35

Blacken elasticsearch filtertransformer

ec1c3b6

Fix all_deps to include _all_ dependencies

5846ea0

Upping lark-parser to 0.7.8 and add ci jobs

5a901c7

Re-introduce requirements.txt with static versions for base dependencies. Create three different dependency GitHub Actions jobs: - static - eager - as user

CasperWA added 2 commits December 4, 2019 12:57

Add flake8 job

e0999e8

Fix GH actions jobs

65367ea

Comment out clean-install and run codecov once

9675300

ml-evs requested review from CasperWA and ltalirz and removed request for ltalirz December 5, 2019 19:16

ml-evs added a commit that referenced this pull request Dec 5, 2019

Update contributing and readme with proposed changes from #92

e295253

CasperWA requested changes Dec 6, 2019

View reviewed changes

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved

This was referenced Dec 6, 2019

Automatically publish version tags to PyPI via GH Actions #107

Merged

Updates to README and docs for v0.10.0 #68

Merged

ml-evs and others added 3 commits December 6, 2019 13:36

Reworked build system

b11df3f

- Restricted versions of backends to major versions in setup.py - Added pinned versions of backends to requirements.txt - Moved requirements.txt to be CI specific

Fix typo in action

5590326

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

Add requirement.txt for eager install

f614d03

Removed pydantic from eager requirements

786cd5e

CasperWA approved these changes Dec 7, 2019

View reviewed changes

CasperWA merged commit ea67905 into master Dec 7, 2019

CasperWA deleted the gh_action_linter branch December 7, 2019 23:15

CasperWA pushed a commit that referenced this pull request Jan 7, 2020

Update contributing and readme with proposed changes from #92

8bc7494

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GitHub Actions for CI #92

Use GitHub Actions for CI #92

ml-evs commented Nov 22, 2019 •

edited

codecov bot commented Nov 22, 2019 •

edited

ml-evs commented Dec 1, 2019 •

edited

CasperWA commented Dec 4, 2019

CasperWA commented Dec 4, 2019

ml-evs commented Dec 4, 2019

CasperWA commented Dec 4, 2019

ml-evs commented Dec 4, 2019 •

edited

CasperWA commented Dec 4, 2019

ml-evs commented Dec 5, 2019 •

edited

CasperWA commented Dec 5, 2019

ltalirz commented Dec 5, 2019

CasperWA commented Dec 5, 2019

ml-evs commented Dec 5, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA left a comment

ml-evs commented Dec 6, 2019

CasperWA commented Dec 6, 2019 •

edited

ml-evs commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

ml-evs commented Dec 7, 2019

CasperWA left a comment

Use GitHub Actions for CI #92

Use GitHub Actions for CI #92

Conversation

ml-evs commented Nov 22, 2019 • edited

codecov bot commented Nov 22, 2019 • edited

Codecov Report

ml-evs commented Dec 1, 2019 • edited

CasperWA commented Dec 4, 2019

CasperWA commented Dec 4, 2019

ml-evs commented Dec 4, 2019

CasperWA commented Dec 4, 2019

ml-evs commented Dec 4, 2019 • edited

CasperWA commented Dec 4, 2019

ml-evs commented Dec 5, 2019 • edited

CasperWA commented Dec 5, 2019

ltalirz commented Dec 5, 2019

CasperWA commented Dec 5, 2019

ml-evs commented Dec 5, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Dec 6, 2019

CasperWA commented Dec 6, 2019 • edited

ml-evs commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

CasperWA commented Dec 6, 2019

ml-evs commented Dec 7, 2019

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Nov 22, 2019 •

edited

codecov bot commented Nov 22, 2019 •

edited

ml-evs commented Dec 1, 2019 •

edited

ml-evs commented Dec 4, 2019 •

edited

ml-evs commented Dec 5, 2019 •

edited

CasperWA commented Dec 6, 2019 •

edited