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

CI: adding remote-data test job, remove duplicate #358

Merged
merged 7 commits into from
Sep 21, 2022

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Sep 17, 2022

This PR reshuffles the CI a bit in order to:

  • add --remote-data=any to one of the jobs.
  • remove the matrix handling of python versions in order to be able to handle the jobs a bit more heterogeneous way

As a result now we test all OSes with all the dependencies while keeping one linux run with the mandatory dependencies only. We also have one remote-data test and one for the oldest dependency versions, and one for the dev versions. I feel with the reshuffle we cover more of our grounds with one less job.

note: I expect the remote-data job to fail until CADC is back online. If it keeps failing after that then we have to deal with a genuine issues.

I would love it if @pllim could have a look at this to whether I've missed a piece of logic somewhere.

@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #358 (a22ee0a) into main (2ef8dde) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   78.36%   78.36%           
=======================================
  Files          46       46           
  Lines        5506     5506           
=======================================
  Hits         4315     4315           
  Misses       1191     1191           
Impacted Files Coverage Δ
pyvo/dal/adhoc.py 66.58% <100.00%> (ø)
pyvo/dal/sia.py 65.44% <100.00%> (ø)
pyvo/dal/ssa.py 55.75% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bsipocz bsipocz force-pushed the CI_add_remote_data branch 2 times, most recently from 0ce01a1 to 648924b Compare September 17, 2022 06:44
@pllim
Copy link
Member

pllim commented Sep 19, 2022

While you are at it, are you interested in using the OpenAstronomy template? @Cadair has a PR out for astropy at astropy/astropy#13641

@pllim
Copy link
Member

pllim commented Sep 19, 2022

Failure looks real?

E       AssertionError: assert 'This data co...bscore table.' == 'This data co...bscore table.'
E         - This data collection is queriable in GAVO Data Center's obscore table.
E         ?                             ^
E         + This data collection is queryable in GAVO Data Center's obscore table.
E         ?     

@msdemlei
Copy link
Contributor

msdemlei commented Sep 19, 2022 via email

@bsipocz
Copy link
Member Author

bsipocz commented Sep 19, 2022

While you are at it, are you interested in using the OpenAstronomy template?

not this time, but thanks for the heads up.

@pllim
Copy link
Member

pllim commented Sep 19, 2022

Hmm, new error now: pyvo.dal.exceptions.DALServiceError: No working capabilities endpoint provided

@bsipocz
Copy link
Member Author

bsipocz commented Sep 19, 2022

Known one, I'm pretty sure that's due to the offline CADC mentioned above. I'll restart CI tomorrow when they are back online, and expect an all-green CI (bar the narrative docs that may have some genuine failures that I'll either fix or skip for now in order to not hold up the release).

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Just looking at the tox and workflow file, things look good. Good luck with the CADC stuff!

@bsipocz
Copy link
Member Author

bsipocz commented Sep 19, 2022

Just looking at the tox and workflow file, things look good. Good luck with the CADC stuff!

thank you Pey Lian!

@andamian
Copy link
Contributor

CADC is slowly coming back after a scheduled maintenance this weekend (big power upgrade to the data centre). sia2 is now failing in a different point... Probably some services have not come up properly yet.

I'm fine with removing the matrix and the allocation of Python versions to targets is logical but in order to preserve that in the ci isn't it better to define the 3 Python versions as maybe older, current and newer so that the next time we slide the version bar we only update these variables? I'm not sure if it's possible to define them at the top of the file and use them throughout.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 20, 2022

3 Python versions as maybe older, current and newer so that the next time we slide the version bar we only update these variables? I'm not sure if it's possible to define them at the top of the file and use them throughout.

I like to think spelling out this explicitly is easier to see right away what's going on, after all I don't think we would save much lines of config with introducing the complexity.

Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

Thanks @bsipocz , this looks good to me as well. Do you want to wait for it to go green before we merge?

@andamian
Copy link
Contributor

I might need to follow up myself on the CADC failure. it could be broken for other reasons but went unnoticed with the remote-data tests not part of the CI.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 20, 2022

Thanks @bsipocz , this looks good to me as well. Do you want to wait for it to go green before we merge?

Yes, keep this until all is green, either as fixed of intentionally skipped.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 20, 2022

@andamian - I see this (or one very much similar) error with cadc in astroquery. Do you want me to open a separate issue somewhere about it?

@pllim
Copy link
Member

pllim commented Sep 20, 2022

If CADC will take a while to resolve, maybe mark it as xfail and move on? If you have xpass strict, it will fail when the service is back up and the test no longer xfail.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 20, 2022

yes, the question now is whether this is an issue with pyvo, because then we should fix it today for the release. If it's fully upstream then I agree, an xfail is a good workaround.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 20, 2022

This one is the example from astroquery running into this, too:

>>> from astropy import units as u
>>> from astroquery.cadc import Cadc
>>> cadc = Cadc()
>>> coords = '01h45m07.5s +23d18m00s'
>>> radius = 0.01*u.deg
>>> images = cadc.get_images(coords, radius, collection='CFHT')

@bsipocz
Copy link
Member Author

bsipocz commented Sep 21, 2022

This got the approval and we discussed the skipping/xfailing of the remaining issues, so I go ahead and merge this now.

@bsipocz bsipocz merged commit 907b164 into astropy:main Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add remote-data testing
5 participants