Refine GitHub Actions #246

andamian · 2020-12-01T07:02:41Z

codecov · 2020-12-01T15:54:01Z

Codecov Report

Merging #246 (5a03412) into master (e87e053) will increase coverage by 2.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
+ Coverage   72.31%   74.83%   +2.52%     
==========================================
  Files          43       43              
  Lines        4515     5035     +520     
==========================================
+ Hits         3265     3768     +503     
- Misses       1250     1267      +17

Impacted Files	Coverage Δ
pyvo/dal/exceptions.py	`76.38% <0.00%> (-5.43%)`	⬇️
pyvo/dal/dbapi2.py	`0.00% <0.00%> (ø)`
pyvo/io/uws/endpoint.py	`100.00% <0.00%> (ø)`
pyvo/utils/decorators.py	`100.00% <0.00%> (ø)`
pyvo/io/vosi/availability.py	`100.00% <0.00%> (ø)`
pyvo/__init__.py
pyvo/_astropy_init.py	`73.52% <0.00%> (ø)`
pyvo/utils/xml/elements.py	`93.43% <0.00%> (+0.06%)`	⬆️
pyvo/dal/params.py	`86.71% <0.00%> (+0.31%)`	⬆️
pyvo/dal/query.py	`85.49% <0.00%> (+0.42%)`	⬆️
... and 15 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 af5ebec...4ba3465. Read the comment docs.

andamian · 2020-12-01T16:22:03Z

Accomplishments in this PR:

moved tests to use pytest command
pull out macOS and Windows out of the matrix into a separate job
added astropy40 and astropy41 to the regression mix
fixed coverage
updated the badge in README

Outstanding:

Is the regression mix appropriate? The actual tests that are being executed are listed in the details of the tests jobs. Please let me know if you want to change anything in there
I don't know how to have the coverage jobs show up in the check list similar to astropy instead of the bot report. Is that a repo config thing?
The CI yml file might be a bit to verbose. Wish there was a way to hide the skipped steps in the report. A different option that I was considering was to configure tox to ignore tests for which the appropriate Python interpreter was missing. That would make the yml file less verbose but the report would be less visible as well (tox report would contain all the regression combinations). Anyways, suggestions are welcome.

pllim

The matrix could be trimmed. Is there a reason why you are testing against astropy 3.x when 4.0.x is the current LTS? I think the important astropy versions to test against are: 4.0.x (LTS), 4.2 (latest stable), and dev

So the matrix could be trimmed down to 3 jobs (any more you add would be a bonus but I am not sure how necessary):

Python 3.7 + astropy 4.0.x + your oldest numpy (1.17?)
Python 3.8 + astropy 4.2 + latest stable numpy
Python 3.9 + astropy dev + latest/dev numpy (dev numpy might be overkill)

The coverage job: Can it be attached to one of the matrix jobs above (maybe stable astropy + stable numpy)? See astropy Actions + Tox setup for example.

Coverage reporting: I do see that it commented on the PR, so coverage upload works. astropy does have a codecov.yml that disables commenting but I don't think that is mutually exclusive with PR status posting. Maybe get this in master first and see if the status then magically appears in subsequent PRs.

Doc build job: I would say it is unnecessary if you opt to use RTD PR builder (which also allows doc preview). Up to you though.

pllim · 2020-12-01T22:24:23Z

.github/workflows/cibuild.yml

@@ -54,10 +55,40 @@ jobs:
    - name: Python 3.8 with astropy32
      if: matrix.python-version == 3.8
      run: tox -e py38-test-astropy32
+    - name: Python 3.8 with astropy40


I guess this works. astropy has a different style if you are interested: https://github.com/astropy/astropy/blob/master/.github/workflows/ci_workflows.yml

Yes, I will do that if we agree to a regression matrix that is symmetrical.

@pllim do you know if astropy releases minor versions to work with newer Python version? E.g. recently released P3.9 expected to work with latest astropy4.0?

astropy 4.0.4 should work with Python 3.9; I see the wheels for Python 3.9 at https://pypi.org/project/astropy/4.0.4/#files

pllim · 2020-12-01T22:26:42Z

.github/workflows/cibuild.yml

@@ -3,6 +3,8 @@ name: CI
 on:
  push:


If you want to prevent people from kicking off CI on their own feature branches in their forks, you can make this stricter.

Suggested change

push:

push:

branches:

- master

tags:

The matrix could be trimmed. Is there a reason why you are testing against astropy 3.x when 4.0.x is the current LTS? I think the important astropy versions to test against are: 4.0.x (LTS), 4.2 (latest stable), and dev

So the matrix could be trimmed down to 3 jobs (any more you add would be a bonus but I am not sure how necessary):

Python 3.7 + astropy 4.0.x + your oldest numpy (1.17?)

Python 3.8 + astropy 4.2 + latest stable numpy

Python 3.9 + astropy dev + latest/dev numpy (dev numpy might be overkill)

The coverage job: Can it be attached to one of the matrix jobs above (maybe stable astropy + stable numpy)? See astropy Actions + Tox setup for example.

Coverage reporting: I do see that it commented on the PR, so coverage upload works. astropy does have a codecov.yml that disables commenting but I don't think that is mutually exclusive with PR status posting. Maybe get this in master first and see if the status then magically appears in subsequent PRs.

Doc build job: I would say it is unnecessary if you opt to use RTD PR builder (which also allows doc preview). Up to you though.

Thanks @pllim
I'm fine with dropping support for 3.x if that's astropy policy. Do we have agreement. As for numpy, as I've mentioned before, for the numpy used in this project I don't think we should bother with different versions. That's dealt with upstream (i.e. astropy)

I'm going to look into attaching coverage to one of the existing jobs in the matrix

How does the RTD builder app work? I haven't used it before.

Any reason not to do CI on feature branches? I use them in my fork and that's why I prefer to see the CI results after a push in a feature branch.

How does the RTD builder app work?

Go to https://readthedocs.org/projects/pyvo/ -> Admin -> Advanced Settings (top left) -> check the "Build pull requests for this project" -> click "Save" (bottom)

Any push to PR after that would trigger a new job from RTD. It is controlled by your https://github.com/astropy/pyvo/blob/master/.readthedocs.yml but you do need to upgrade that YAML to RTD v2 first (https://docs.readthedocs.io/en/stable/config-file/v2.html). You also have the option to let it fail if there is any warning or not; up to you.

Any reason not to do CI on feature branches?

To save the Earth a little, I guess. But if you think those extra runs are necessary, then keep it as-is.

As for dropping matrix for astropy 3.x, that is up to you. I don't see any minversion of astropy defined in your setup.cfg. Usually you want to test against the minversion that you say you support.

Is there a way you can ask PyVO stakeholders about this? No one should still be using Python 2 or 3.5, so I don't see why anyone needs astropy<4.

I think that astroquery is the main stakeholder so I'll wait for @bsipocz word.

Can ping @keflavich and @ceb8 too.

I don't think we rely on astropy < 4; is that the question? haven't time to read back through whole convo now

Yes, that was the question. In this case, we'll drop support for 3.*

README.rst

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

tomdonaldson · 2020-12-02T19:53:31Z

.github/workflows/cibuild.yml

@@ -3,6 +3,8 @@ name: CI
 on:
  push:


I agree with @pllim's suggestion on trimming the matrix, and see no reason to do anything with astropy 3.x. I would even be OK with us specifying astropy>=4.1 in setup.cfg since that's not a big ask from our users and gives us consistent handling of string values in VOTables (no more byte strings).

For our convenience, I do like triggering CI on feature branches. It is overkill, but I have found it to save me headaches and dev time by catching weird things sooner. That helps me rationalize the extra runs along with the fact that the pyvo test suite doesn't take long at all to run.

I yield to @pllim's opinion on the other matters, and admit to not fully grasping the details of the macOS and Windows build specs, but am happy to see how they run.

andamian · 2020-12-04T07:12:22Z

Another version with trimmed matrix is now available.

tomdonaldson · 2020-12-04T16:04:15Z

.github/workflows/cibuild.yml

@@ -3,6 +3,8 @@ name: CI
 on:
  push:
  pull_request:
+  schedule:


I've just noticed this section. What is the intended effect of this cron? I'm new to this syntax so it looks like this scheduling is independent of the other triggers (so the CI would run every 3 hours all the time), but I could also imagine it somehow chaining with the push and pull_request to bin the CI runs once those triggers happen. Are there reasons to run other than when a push or pull_request trigger happens?

I think that spec tells it to run daily at 3 AM (UTC). If this is a low-traffic package, weekly might work too.

Cron job is to catch upstream changes that might break your code. So I don't think you should get rid of it. Most of the time, it would pass, but for that rare case a new upstream release breaks your code, you would want to know even when there is no push to master. Most common example is that a new release finally removed that deprecated API and you have been ignoring the deprecation warning; or it removed/changed a hidden function that you have been using. Hope that clarifies the matter.

The nightly build cron runs at 3:00UTC in the master.

Yes, sorry, I don't know why I read that as every 3 hours. Daily seems OK to me. It's nice to see those upstream impacts sooner rather than later.

Yeah, I'm also nervous about build breakages due to outside things, even outside astropy, as sometimes it's a while between PRs.

.github/workflows/cibuild.yml

.readthedocs.yml

pllim · 2020-12-04T16:16:09Z

README.rst

-.. image:: https://travis-ci.org/astropy/pyvo.svg
-    :target: https://travis-ci.org/astropy/pyvo
-    :alt: Travis Status
+.. image:: https://github.com/astropy/pyvo/workflows/CI/badge.svg?branch=master&event=schedule


Why event=schedule?

Because it reflects the (more or less current) status of the master branch. Where should we point it to?

Maybe https://github.com/astropy/pyvo/workflows/CI/badge.svg?branch=master is enough so it picks up merge commit runs too?

Yeah, you have a good point here. I'll update it.

pllim · 2020-12-04T16:16:53Z

tox.ini

-    astropy30: with astropy 3.0.*
-    astropy31: with numpy 3.1.*
-    astropy32: with numpy 3.2.*
+    astropy40: with astropy 4.0.*


You might also want to match your astropy minversion to your numpy minversion for cross-testing.

Sorry, I don't understand. We don't have min versions specified and we've decided not to do cross-testing here, no?

In that case, I still recommend cross testing with the minversion of numpy that astropy 4.0 supports (numpy 1.16), but if you disagree, feel free to ignore; this is just a recommendation.

pllim · 2020-12-04T16:18:10Z

p.s. Weird, I don't see the RTD build here. Did you enable it on the RTD side?

pllim · 2020-12-04T16:19:00Z

p.p.s. Is egginfo test still necessary?

pllim · 2020-12-04T16:22:09Z

p.p.p.s.

Test header won't show unless you move https://github.com/astropy/pyvo/blob/master/pyvo/conftest.py to root directory. Some people say that messes up the test runner (i.e., mypackage.test()) but not seeing it in CI also isn't very useful.

Also, I was thinking more like 3 jobs instead of 9 jobs.

Python 3.7 + astropy 4.0
Python 3.8 + astropy 4.1
Python 3.9 + astropy 4.2 (latest)

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

andamian · 2020-12-04T16:34:39Z

p.s. Weird, I don't see the RTD build here. Did you enable it on the RTD side?

I thought I did.

andamian · 2020-12-04T16:37:33Z

p.p.s. Is egginfo test still necessary?

It was there from before so I just preserved it. I assume it was meant to do a quick check before spawning all the test jobs.

pllim · 2020-12-04T16:44:48Z

Re: egginfo -- Ah, okay.

Re: RTD -- Ops, I forgot. The webhook setting in this repo also has to be updated. I have updated it for you. So theoretically, in your next push, it should kick off. 🤞

andamian · 2020-12-04T16:50:44Z

p.p.p.s.

Test header won't show unless you move https://github.com/astropy/pyvo/blob/master/pyvo/conftest.py to root directory. Some people say that messes up the test runner (i.e., mypackage.test()) but not seeing it in CI also isn't very useful.

What is the test header?

Also, I was thinking more like 3 jobs instead of 9 jobs.

Python 3.7 + astropy 4.0

Python 3.8 + astropy 4.1

Python 3.9 + astropy 4.2 (latest)

Really? 3 doesn't make a matrix :-).
Do you think there are too many jobs or some combinations are not meaningful? I can see for example that Py3.9 + Astropy 4.0 might not be that relevant since a user that just upgraded to Py3.9 is likely going to get the latest version of the libraries but that is not always the case. And it's perfectly reasonable for Py3.8 users to want to upgrade to latest astropy, no?
If the problem is the number of jobs, we could do the regression with tox so that there will be 3 jobs but each of them will test with 3 versions of astropy. It will be less clear what failed and why. What do you think?

andamian · 2020-12-04T16:51:48Z

Re: egginfo -- Ah, okay.

Re: RTD -- Ops, I forgot. The webhook setting in this repo also has to be updated. I have updated it for you. So theoretically, in your next push, it should kick off. 🤞

Awesome! Thanks!

pllim · 2020-12-04T17:02:04Z

What is the test header?

https://github.com/astropy/pytest-astropy-header ; it reports the dependency versions. I see that you have it set up. It is just not displaying in the CI log.

Do you think there are too many jobs or some combinations are not meaningful?

It all depends what you want to test, I guess. Is it really PyVO's problem when astropy 4.0 suddenly breaks in Python 3.9? Do you need to test all those combos for every commit on a PR?

I can see that it might be useful for cron, but might be a little excessive for pull request? But if you disagree, feel free to keep it as is. I'll leave that to your good judgement.

andamian · 2020-12-04T17:33:10Z

What is the test header?

https://github.com/astropy/pytest-astropy-header ; it reports the dependency versions. I see that you have it set up. It is just not displaying in the CI log.

Got it. I've moved it to root - hopefully this helps.

It all depends what you want to test, I guess. Is it really PyVO's problem when astropy 4.0 suddenly breaks in Python 3.9? Do you need to test all those combos for every commit on a PR?

I can see that it might be useful for cron, but might be a little excessive for pull request? But if you disagree, feel free to keep it as is. I'll leave that to your good judgement.

I agree with you that it might be excessive but pyvo is relatively lightweight and tests run fast. For now, I would keep it in this form unless the others have a different opinion. Later we can revisit it and trim it down if necessary or split the flow into a ci and a nightly for ex. Good point nevertheless. Thanks @pllim

andamian · 2020-12-04T18:27:05Z

@pllim - good news is that RTD kicked, the bad news is that it failed. Do you happen to know why sphinx_astropy does not get installed? Where should I add it as a dependency?

.readthedocs.yml

conftest.py

andamian · 2020-12-07T17:34:31Z

@tomdonaldson, @cbanek & @bsipocz - This is ready for the final review. @pllim has helped me iron out the last details (many thanks!). Please review at your earliest convenience or let me know if you are OK with me releasing it. There is at least another PR that is blocked on the CI and I'd like to get back to the user soon. Thanks.

tomdonaldson · 2020-12-07T17:46:55Z

@tomdonaldson, @cbanek & @bsipocz - This is ready for the final review. @pllim has helped me iron out the last details (many thanks!). Please review at your earliest convenience or let me know if you are OK with me releasing it. There is at least another PR that is blocked on the CI and I'd like to get back to the user soon. Thanks.

This all looks fine to me. Thanks very much @pllim for the discussion and guidance. I've learned a lot.

cbanek

Looks good to me, so many thanks to @andamian for doing all the work and @pllim for all the great feedback. I echo @tomdonaldson that I learned a lot!

pllim

The test header is still not appearing but it can be fixed as a separate PR, because I think you might as well move the dependencies metadata to setup.cfg and apply APE 17 while you are at it.

Thanks for your hard work on this!

p.s. You are welcome! Glad to help.

andamian added the enhancement label Dec 1, 2020

andamian added this to the v1.2 milestone Dec 1, 2020

andamian self-assigned this Dec 1, 2020

andamian added the no-changelog-entry-needed label Dec 1, 2020

andamian marked this pull request as draft December 1, 2020 07:17

Initial attempt

510c4a4

andamian force-pushed the actions2 branch from 4b40e97 to 510c4a4 Compare December 1, 2020 15:56

andamian marked this pull request as ready for review December 1, 2020 16:09

andamian requested review from bsipocz, cbanek, pllim and tomdonaldson December 1, 2020 16:22

pllim reviewed Dec 1, 2020

View reviewed changes

Update README.rst

2d346bd

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

tomdonaldson requested changes Dec 2, 2020

View reviewed changes

Changes after code review

0d84449

andamian force-pushed the actions2 branch from ae90b8d to 0d84449 Compare December 4, 2020 07:03

tomdonaldson reviewed Dec 4, 2020

View reviewed changes