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

Re-enable mamba in CI #3177

Merged
merged 9 commits into from Apr 21, 2021
Merged

Re-enable mamba in CI #3177

merged 9 commits into from Apr 21, 2021

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Mar 16, 2021

Fixes #3048

Changes made in this Pull Request:

  • Enable mamba for GH actions CI

Testing on some cron jobs I'll push to MDA later, it looks like it works now (not sure if it's that stable, but I guess we can always disable again if it fails).

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@IAlibay
Copy link
Member Author

IAlibay commented Mar 16, 2021

The gains here are modest (~ minute and a half).

Personally I think we should go ahead with it anyways, it adds up to ~ 10 mins of CPU time which is a non-insignificant environmental impact.

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #3177 (4154695) into develop (8a5ef48) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3177   +/-   ##
========================================
  Coverage    92.81%   92.81%           
========================================
  Files          170      170           
  Lines        22801    22801           
  Branches      3239     3239           
========================================
  Hits         21162    21162           
  Misses        1591     1591           
  Partials        48       48           

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 8a5ef48...4154695. Read the comment docs.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

LGTM.
By the way, mdtraj has nuked conda/mamba for nix. That could be something we could look at if installation remains slow.

https://github.com/mdtraj/mdtraj/blob/master/.github/workflows/main.yaml

@IAlibay IAlibay marked this pull request as draft April 16, 2021 09:54
@IAlibay
Copy link
Member Author

IAlibay commented Apr 16, 2021

Thanks @lilyminium I'm going to temporarily switch to draft though because I think my latest commit caused a performance regression.

By the way, mdtraj has nuked conda/mamba for nix. That could be something we could look at if installation remains slow.

Yeah, for now we should be fine, we're sub 1 minute for dependency installs on linux. Annoyingly MacOS takes much longer.

@orbeckst
Copy link
Member

nix looks interesting and is available for Linux and macOS.

Is it a disadvantage to recommend, say, conda for users but use another package manager for the bulk of our testing?

@IAlibay
Copy link
Member Author

IAlibay commented Apr 21, 2021

Alright, I think this is as optimised as I can get it.

Main changes are:

  • addition of mamba in places where we do lots of dependency installs
  • setting the number of macOS processors to 3 instead of 2 (this doesn't have a noticeable influence since the performance differs a lot between runs, but in some cases it can make it run a bit faster)
  • Switching the channel priorities around (which is something that's being introduced in the chemfiles PR)

Quick timing summary:

Linux:

full: ~ 2 mins less compared to current develop

miniconda setup: ~ 1 min
deps install: ~ 1 min

min: ~ 1 min less compared to current develop

miniconda_setup: ~ 1 min
deps install: ~ 20s

build_docs: ~ 3 mins less compared to current develop

setup_miniconda: ~ 1 min
install_deps: ~ 1 min

pylint: ~ same as current develop

setup_miniconda: 46s
deps install: 5s

pypi_check: same as current develop

setup_miniconda: 32s
deps install: 10s

macOS:

full: for setup ~ 7 mins less compared to current develop

setup_miniconda: 1m 9s
install_deps: 1m 43s
run_tests: ~ 15 - 25 mins (same as current develop? sometimes faster?)

@IAlibay IAlibay marked this pull request as ready for review April 21, 2021 19:18
@IAlibay IAlibay requested a review from lilyminium April 21, 2021 19:18
Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for commenting why it's a conda install in pypi_check.

@lilyminium lilyminium merged commit ec7d636 into MDAnalysis:develop Apr 21, 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.

Enable mamba in github actions CI
3 participants