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: use nightly wheels #353

Merged
merged 8 commits into from
Sep 14, 2022
Merged

CI: use nightly wheels #353

merged 8 commits into from
Sep 14, 2022

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Sep 11, 2022

This PR does a few minor CI improvements:

  • switches to use "nightly" wheels for dev versions rather than building from source
  • opt-in to also include testing with the dev numpy
  • makes sure that we actually test with the mandatory dependencies only (previously all dependencies were always installed)
  • run coverage test after a test suite run rather than after running the linter (as that one shouldn't install dependencies)
  • minor cleanups to ensure the relevant versions are printed in the test header

commands =
devdeps: pip install -U --pre --only-binary :all: -i https://pypi.anaconda.org/scipy-wheels-nightly/simple numpy
Copy link
Member

Choose a reason for hiding this comment

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

Stuart proposed another way to do this over at astropy/astropy#13614

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, interesting. That was more like the first approach we had, and then we switched over to having these individual pip installs. I think Stuart's version is cleaner in the case when you have multiple packages. However, here we only use the scipy-wheels-nightly for numpy, and something else for astropy, and don't install other packages. So I would say we should keep it as a one-liner like this rather than adding extras.

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 very little to choose between them, so use your favourite, but sunpy uses multiple index URLs with the same pattern https://github.com/sunpy/sunpy/blob/main/tox.ini#L34

tox.ini Outdated
commands =
pip freeze
pytest --pyargs pyvo --cov pyvo --cov-config={toxinidir}/setup.cfg
coverage xml -o {toxinidir}/coverage.xml
python setup.py egg_info
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? I think astropy stopped testing this a long time ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I suppose it can go.

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.

At a glance, LGTM.

You would want to inspect the logs carefully to make sure each job is running on the versions you think they are running on. Nice work!

@andamian
Copy link
Contributor

Great work @bsipocz - the CI is faster by an order of 3 or 4. Awesome!

Coverage fails to upload. Wondering if it's because of the missing Graphviz dependency

The point in the description ("makes sure that we actually test with the mandatory dependencies only (previously all dependencies were always installed)") - I couldn't figure out where/how it's addressed

The rest looks good to me. Thanks!

@bsipocz
Copy link
Member Author

bsipocz commented Sep 13, 2022

The point in the description ("makes sure that we actually test with the mandatory dependencies only (previously all dependencies were always installed)") - I couldn't figure out where/how it's addressed

haha, it actually helped me to realize that there were some syntax mistakes done while clearing up tox. So, I expect things won't be as fast now.

@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #353 (488072f) into main (82bb4a4) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   78.28%   78.27%   -0.01%     
==========================================
  Files          46       46              
  Lines        5494     5497       +3     
==========================================
+ Hits         4301     4303       +2     
- Misses       1193     1194       +1     
Impacted Files Coverage Δ
pyvo/__init__.py 100.00% <ø> (ø)
pyvo/dal/params.py 86.38% <0.00%> (-0.34%) ⬇️
pyvo/dal/query.py 85.16% <0.00%> (+0.03%) ⬆️
pyvo/dal/tap.py 69.49% <0.00%> (+0.08%) ⬆️

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

@bsipocz
Copy link
Member Author

bsipocz commented Sep 13, 2022

This should now fix and close #354. Note that mentioning mimeparse has been removed from docs and config as I see it is in fact not being used (and hasn't been release either in the past ~9 years)

@bsipocz
Copy link
Member Author

bsipocz commented Sep 14, 2022

OK, so picked up versions seem to be correct, this now only needs an approval and ready to go in.

@andamian
Copy link
Contributor

It looks good to me, but now that the CI speed gains seem to be gone it begs the question of what is the point of using wheels (and add an extra dependency on anaconda and the process that publishes the wheels) in this particular case?

To be clear: the cleanup done here is still most welcome.

@pllim
Copy link
Member

pllim commented Sep 14, 2022

CI speed gains seem to be gone

What do you mean? The dev job now only takes 1 min. The dev job in main takes 3 mins.

@andamian
Copy link
Contributor

CI speed gains seem to be gone

What do you mean? The dev job now only takes 1 min. The dev job in main takes 3 mins.

I might compare apples with oranges here but the total execution time improved from 5.27min to 5.20min. Certainly not the big jump that I saw initially.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 14, 2022

It looks good to me, but now that the CI speed gains seem to be gone it begs the question of what is the point of using wheels (and add an extra dependency on anaconda and the process that publishes the wheels) in this particular case?

The whole point here is to avoid building astropy and numpy from source, yet test with the dev version.
Anaconda is only providing the storage location, they are not at all involved in the process of producing and publishing these wheels. So the system is pretty reliable, as well as the standard process by the big libraries. So I wouldn't worry about this.

As for CI speed improvement is gone, yep, there was a syntax mistake so the tests were not run in fact, so no wonder the jobs were faster 😊

@pllim
Copy link
Member

pllim commented Sep 14, 2022

improved from 5.27min to 5.20min

That makes sense to me. Building from source isn't that long to begin with. A bulk of the actual run time is setting up the test env, running tests, and then the reporting.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 14, 2022

(oh, and I'm not sure where that 5 mins is coming from, it has some parallelization included already. If you click on it, it's 11 mins vs 16-13 min of total runtime.)

@bsipocz
Copy link
Member Author

bsipocz commented Sep 14, 2022

improved from 5.27min to 5.20min

That makes sense to me. Building from source isn't that long to begin with. A bulk of the actual run time is setting up the test env, running tests, and then the reporting.

Note that we also use numpy-dev atm, and that shouldn't not be built from source.

@andamian
Copy link
Contributor

All good! It was probably just that the initial (buggy) version set the expectations too high. :-). Thanks for the improvements.

@bsipocz bsipocz merged commit 861298f into astropy:main Sep 14, 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.

None yet

4 participants