Skip to content

Switch CI install to mamba#1073

Merged
dwhswenson merged 6 commits into
openpathsampling:masterfrom
dwhswenson:mamba3
Sep 23, 2021
Merged

Switch CI install to mamba#1073
dwhswenson merged 6 commits into
openpathsampling:masterfrom
dwhswenson:mamba3

Conversation

@dwhswenson

@dwhswenson dwhswenson commented Sep 22, 2021

Copy link
Copy Markdown
Member

Third (and, I hope, final!) version of this. See also #1072 and #1071 for the history of trying to debug this problem.

Main points, quoting from previous:

Py27 builds have taken too long for a while, and now they're so bad that they're failing CI. (20+ minutes was bad, but then it jumped to 2 hours -- and now it takes more than 6 hours?!?!?!) I don't know why this is getting worse, but this is insane.

In this PR, I'm going to try to switch to mamba. I've personally been using mamba for a while now. I tested locally and could build a Py27 environment for OPS in a few minutes, using the same command that hangs in CI. (The conda version of the same command is still trying to run -- the problem isn't with GitHub actions, since it affects me, too.)

However, that change resulted in a segfault that appears to have been because a test didn't close a storage before removing the file (which is fixed in this PR).

In addition to fixing that segfault and switching to mamba for installation, this PR:

  • Adds a commented debugging step (allowing one to SSH into the worker) to the main CI workflow.
  • Improves the requirements installation in several ways, including giving a way to override/replace conda installs with pip installs, and using a more consistent (and purely conda-forge) installation procedure.

sroet
sroet approved these changes Sep 22, 2021
View reviewed changes

@sroet sroet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, one leftover question. Feel free to merge without ;)

Comment thread devtools/conda_install_reqs.sh Outdated
if [ ! -z "$OPS_ENV" ]
then
conda create -q -y --name $OPS_ENV conda future pyyaml python=$CONDA_PY
$INSTALL_CMD --name $OPS_ENV conda future pyyaml python=$CONDA_PY

@sroet sroet Sep 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

only leftover comment from the other PR:

"Is the conda install here necessary?

Suggested change
$INSTALL_CMD --name $OPS_ENV conda future pyyaml python=$CONDA_PY
$INSTALL_CMD --name $OPS_ENV future pyyaml python=$CONDA_PY

@codecov

codecov Bot commented Sep 22, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1073 (467968c) into master (a760afe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1073   +/-   ##
=======================================
  Coverage   81.57%   81.57%           
=======================================
  Files         140      140           
  Lines       15416    15416           
=======================================
  Hits        12576    12576           
  Misses       2840     2840

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 a760afe...467968c. Read the comment docs.

@dwhswenson
update conda_install_reqs for more modern usage
8ccc108 
There was some stuff in there that went back to parsing conda
recipees, which is now far from relevant.
@dwhswenson
correct conditions for conda activate
467968c
@dwhswenson

dwhswenson commented Sep 22, 2021

Copy link
Copy Markdown
Member Author

I restructured the part in question a little more thoroughly than just the recommended change. That if statement was from a very old way of generating our list of requirements; we no longer need pyyaml (nor future, since even in the Py2 tests, this script is only run from Py3 -- the Py2 environment would be created by this script.)

The script is used by the conda_ops_dev_install script, though, so a few pieces of functionality need to be left in (like creating a new environment).

@sroet : If you have a chance to take another look, please do. If not, I'm pretty sure the changes are fine, and I'll aim to merge this before the next overnight scheduled tests run.

sroet
sroet approved these changes Sep 23, 2021
View reviewed changes

@sroet sroet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@dwhswenson

dwhswenson commented Sep 23, 2021

Copy link
Copy Markdown
Member Author

Whoops. forgot to merge this before I went to bed last night!

@dwhswenson dwhswenson merged commit d28571f into openpathsampling:master Sep 23, 2021
@dwhswenson dwhswenson deleted the mamba3 branch September 23, 2021 10:24
@dwhswenson dwhswenson mentioned this pull request Sep 30, 2021
Closed
@dwhswenson dwhswenson mentioned this pull request Oct 22, 2021
Merged
@dwhswenson dwhswenson mentioned this pull request Jan 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sroet sroet sroet approved these changes

Assignees

No one assigned

Labels

misc PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dwhswenson @sroet