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

Update CI to Github Actions #471

Merged
merged 2 commits into from
Oct 2, 2020
Merged

Update CI to Github Actions #471

merged 2 commits into from
Oct 2, 2020

Conversation

bradyrx
Copy link
Collaborator

@bradyrx bradyrx commented Sep 29, 2020

Description

Updates CI to Github Actions.

Closes #454

To-Do

  • Add install tests for different operating systems (but not tests)

@aaronspring aaronspring mentioned this pull request Sep 29, 2020
14 tasks
@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 29, 2020

Okay there's ways to install conda with one of their already installed miniconda setups: https://github.com/marketplace/actions/setup-conda

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 29, 2020

FYI before we merge this we need to change the "branches" to either "master" or some variable for current branch. I made it dummy3 so it would immediately run on this.

@aaronspring
Copy link
Collaborator

Fails very uninformative...

Error: The operation was canceled.

@aaronspring
Copy link
Collaborator

Although for py37 esmpy is the problem. https://github.com/pangeo-data/climpred/pull/471/checks?check_run_id=1184207681

@aaronspring
Copy link
Collaborator

Making xesmf and esmpy optional should also happen. Just covers the smoothing. Actually .interp_like also does a decent job

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 29, 2020

See second tol ast comment JiaweiZhuang/xESMF#47 (comment). I think if you install xesmf before esmpy it should work.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 29, 2020

That ordering works

name: xesmf_v3
channels:
  - conda-forge
  - defaults
dependencies:
  - xesmf
  - esmpy
  - dask

@aaronspring
Copy link
Collaborator

We should switch to pangeo xesmf anyways but they are not on pypi

@aaronspring
Copy link
Collaborator

Does pip install esmpy not work? Looks like that. Also pip not included here http://www.earthsystemmodeling.org/esmf_releases/non_public/ESMF_7_0_0/esmpy_doc/html/install.html#installing-esmpy

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 29, 2020

Yeah we should go ahead and switch over to that conda plugin for actions I linked. We can have a testing.yml req file with this minimum number of packages we set up here. I'm happy to do that tomorrow or Thursday unless yo uwant to try now.

@aaronspring
Copy link
Collaborator

Maybe calling it minimum.yml

@bradyrx bradyrx changed the title fix readme Update CI to Github Actions Sep 30, 2020
@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 30, 2020

Picking this up from where you left off.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 30, 2020

Need to switch to CodeCov. See coverallsapp/github-action#30. Coveralls doesn't integrate well here. Testing that now..

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 30, 2020

Looks like actually test_versions was breaking with main(). Need to check that htat's necessary. I don't think that's ever called since the file isn't called directly. People just use print_versions

@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #471 into master will increase coverage by 94.32%.
The diff coverage is 94.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #471       +/-   ##
===========================================
+ Coverage    0.00%   94.32%   +94.32%     
===========================================
  Files          12       50       +38     
  Lines        1040     4760     +3720     
===========================================
+ Hits            0     4490     +4490     
+ Misses       1040      270      -770     
Impacted Files Coverage Δ
climpred/metrics.py 93.22% <ø> (+93.22%) ⬆️
climpred/preprocessing/mpi.py 25.00% <25.00%> (ø)
climpred/tests/test_preprocessing.py 53.73% <53.73%> (ø)
climpred/preprocessing/shared.py 60.00% <60.00%> (ø)
climpred/versioning/print_versions.py 61.33% <61.33%> (ø)
climpred/testing.py 80.76% <80.76%> (ø)
climpred/tests/test_metrics.py 81.57% <81.57%> (ø)
climpred/relative_entropy.py 82.69% <85.71%> (+82.69%) ⬆️
climpred/smoothing.py 87.50% <87.50%> (ø)
climpred/tutorial.py 79.03% <87.50%> (ø)
... and 83 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 6625e0a...57ffe35. Read the comment docs.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Oct 1, 2020

All done in parallel so those all ran fast. Gotta figure out if I need both "push" and "pull request"

@aaronspring
Copy link
Collaborator

it runs so fast. I dont think pytest is running

@bradyrx
Copy link
Collaborator Author

bradyrx commented Oct 1, 2020

it runs so fast. I dont think pytest is running

It is running. Click "details" then click the "run tests" dropdown. We don't install all the extra packages and so on with this new environment.

@aaronspring
Copy link
Collaborator

Do we need to test against py36 37 and 38? Isn’t one enough?

@bradyrx
Copy link
Collaborator Author

bradyrx commented Oct 1, 2020

Do we need to test against py36 37 and 38? Isn’t one enough?

Yes I think we definitely should. New versions could cause breaking changes we're unaware of. Most other big packages test against these versions. I think it's fine anyways since they're being run in parallel so it isn't a loss of time for us.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Oct 1, 2020

Also, the linter is breaking on flake8 with style issues that don't exist for me.. I'm confused.

@aaronspring
Copy link
Collaborator

Do we need to test against py36 37 and 38? Isn’t one enough?

Yes I think we definitely should. New versions could cause breaking changes we're unaware of. Most other big packages test against these versions. I think it's fine anyways since they're being run in parallel so it isn't a loss of time for us.

I think with these multiple env tests we absolutely make the downloads number unreliable. but probably CI is already the majority of downloads. I just dont see which breaking change could come and why we need to test every PR against these. I understand that we should do this once in a while (e.g. before releasing)

@bradyrx
Copy link
Collaborator Author

bradyrx commented Oct 1, 2020

Do we need to test against py36 37 and 38? Isn’t one enough?

Yes I think we definitely should. New versions could cause breaking changes we're unaware of. Most other big packages test against these versions. I think it's fine anyways since they're being run in parallel so it isn't a loss of time for us.

I think with these multiple env tests we absolutely make the downloads number unreliable. but probably CI is already the majority of downloads. I just dont see which breaking change could come and why we need to test every PR against these. I understand that we should do this once in a while (e.g. before releasing)

We aren't downloading climpred in the CI. It's now installing it via minimum-tests which looks like this:

name: climpred-minimum-tests
channels:
  - conda-forge
  - defaults
dependencies:
  - xesmf
  - esmpy
  - dask
  - netcdf4
  - ipython
  - nc-time-axis
  - coveralls
  - pytest
  - pytest-cov
  - pip
  - pip:
    - pytest-lazy-fixture
    - -e ../..

The last line installs climpred via pip locally.

@bradyrx bradyrx force-pushed the dummy3 branch 2 times, most recently from 42ff849 to 41778fd Compare October 1, 2020 16:01
@bradyrx bradyrx requested a review from aaronspring October 1, 2020 16:18
@bradyrx bradyrx self-assigned this Oct 1, 2020
@@ -0,0 +1,22 @@
name: climpred installs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Worth adding this. Found out if you install climpred from scratch it doesn't work without ipython and toolz installs.

Copy link
Collaborator

@aaronspring aaronspring left a comment

Choose a reason for hiding this comment

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

This is a great improvement. Hard work and very good for our project here.

@bradyrx bradyrx merged commit 80c17f0 into master Oct 2, 2020
@bradyrx bradyrx deleted the dummy3 branch October 2, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strip requirements to minimum
2 participants