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

Bumped CI dependencies #279

Merged
merged 11 commits into from
Dec 17, 2021
Merged

Bumped CI dependencies #279

merged 11 commits into from
Dec 17, 2021

Conversation

andamian
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #279 (6eb1a69) into master (c2bf90c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #279   +/-   ##
=======================================
  Coverage   75.51%   75.51%           
=======================================
  Files          44       44           
  Lines        5129     5129           
=======================================
  Hits         3873     3873           
  Misses       1256     1256           

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 c2bf90c...6eb1a69. Read the comment docs.

@andamian andamian marked this pull request as draft November 26, 2021 22:31
@andamian
Copy link
Contributor Author

andamian commented Nov 26, 2021

This is in preparation for the 1.2 release that astroquery and users are asking for. Please comment on the ranges (cc @bsipocz ).

@andamian andamian marked this pull request as ready for review November 26, 2021 23:02
tox.ini Outdated
@@ -1,6 +1,6 @@
[tox]
envlist =
py{37,38,39}-{latest,astropy40,astropy41}
py{37,38,39,310}-{latest,astropy42,astropy43,astropy50}
Copy link
Member

Choose a reason for hiding this comment

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

you need to define dependency rules for these new versions below.
Also, do you plan to drop astropy 4.0 support?

Either case, what I would suggest is to have one called oldestdeps or similar, that always have the same versions as you have in the package configs as minimum required dependencies to ensure the package is actually compatible with that version combination. And then also to have one job that runs with all on the latest-and-greatest. And whether you test with everything in between is less critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds reasonable.

It looks like 4.0 doesn't work with Python 3.10 so I had to drop it.

There's currently no minimal astropy version in setup.cfg but I will add it. I'm not sure how to centralize that oldestdeps/latestdeps info in one place. It is used in setup.cfg, tox.ini and GH Actions. Do you already have an example of how to achieve that?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, we never assume that all combo will work, e.g. 4.0 shall work with all the versions that were the recent ones when it was released. It's never an expectation that it works with the current latest and greatest. (E.g. oldest combo could be astropy 4.0 with numpy 1.16 etc, even though those are not 3.10 compatible. But then of course dropping them is reasonable, too. NEP-29 (https://numpy.org/neps/nep-0029-deprecation_policy.html) and SPEC-0 are good rule of thumb to follow, e.g. don't drop versions before those timelines, but it's fair game once something is as old as in those guidelines: https://scientific-python.org/specs/spec-0000/)

Copy link
Member

Choose a reason for hiding this comment

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

There's currently no minimal astropy version in setup.cfg but I will add it. I'm not sure how to centralize that oldestdeps/latestdeps info in one place. It is used in setup.cfg, tox.ini and GH Actions. Do you already have an example of how to achieve that?

You can use e.g. astroquery as an example. setup.cfg sets the minimum supported. I like to set it even when it's not strictly necessary, but think about it as a guarantee that those versions would work. Then they need to be repeated in tox.ini to make sure they are indeed tested and work together.
The nice thing with tox is that then you don't need to repeat the versions anymore in GHA (or any other CI you may use), just use the name of the job, and update the versions for that job in tox whenever needed.

@andamian andamian marked this pull request as draft December 1, 2021 19:48
@stargaser
Copy link
Contributor

Hi, for the NAVO pyvo workshop at AAS 239 in Salt Lake City, we want to freeze the content and installation instructions on December 15th. That's one week away. What else is needed for this PR to complete?

cc @tomdonaldson

@andamian
Copy link
Contributor Author

andamian commented Dec 9, 2021

@bsipocz I think this makes it more clear - thanks for the suggestions. Next time we will re-work the workflow file to simplify it using astroquery format but for now this achieves its intent.

@andamian andamian marked this pull request as ready for review December 9, 2021 16:17
Comment on lines +31 to 32
astropy
requests
Copy link
Member

Choose a reason for hiding this comment

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

These should be picked up from the setup file, don't need to list them out in tox (unless of course you want to set a version number for them), but you may need to add an [options] section, too (I'm not sure whether dependencies would be properly picked up from [metadata])

@tomdonaldson tomdonaldson merged commit 475897f into astropy:master Dec 17, 2021
@bsipocz bsipocz mentioned this pull request Dec 17, 2021
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.

None yet

5 participants