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

Source canonical Nextstrain environment file for Travis CI #421

Closed
wants to merge 2 commits into from

Conversation

trvrb
Copy link
Member

@trvrb trvrb commented Dec 11, 2019

Via @huddlej:

The canonical environment file is the most up-to-date and does not include conda-forge as a channel. By sourcing this file, we should always get the most recently environment for nextstrain and, in this case, avoid Travis CI timeouts related to long environment solving with conda-forge.

The canonical environment file is the most up-to-date and does not include
conda-forge as a channel. By sourcing this file, we should always get the most
recently environment for nextstrain and, in this case, avoid Travis CI timeouts
related to long environment solving with conda-forge.
@trvrb
Copy link
Member Author

trvrb commented Dec 11, 2019

Should we remove the environment.yml file from augur in this case? I feel like we should either depend on Augur conda environment for tests, etc... or push everything to nextstrain.yml.

I would think probably safer to just be maintaining a single environment at https://github.com/nextstrain/conda?

Note that we reference environment.yml here: https://nextstrain-augur.readthedocs.io/en/stable/installation/installation.html#using-conda and nextstrain.yml here: https://nextstrain.org/docs/getting-started/local-installation#install-augur--auspice-with-conda-recommended

Switching from environment.yml to nextstrain.yml adds ~40 seconds to Conda install as we wait for pandas, nodejs, cvxopt and git.

@tsibley
Copy link
Member

tsibley commented Dec 11, 2019

I think it'd be reasonable to depend on the canonical Nextstrain Conda environment, but note that the Travis setup will have to explicitly re-install augur into the environment from the checked out source. Otherwise I believe the environment will include the latest released augur, not the commit being tested.

@huddlej
Copy link
Contributor

huddlej commented Dec 11, 2019

Isn't the explicit reinstall already happening with the pip3 install -e .[dev] command? I'm not familiar with Travis CI, but I assumed that the creation of the nextstrain environment in the before_install block of the CI config would precede the pip install command from the repo.

@tsibley
Copy link
Member

tsibley commented Dec 11, 2019

Oh I totally missed that! Yes, you are correct @huddlej.

@tsibley
Copy link
Member

tsibley commented Dec 11, 2019

I had seen that the environment.yml specified

pip:
  - .

and thought Travis was relying on that.

This removes the `environment.yml` in the repository in favor of the "canonical" `nextstrain.yml` maintained at https://github.com/nextstrain/conda. This also updates the install docs to match.
@trvrb
Copy link
Member Author

trvrb commented Dec 14, 2019

@rneher --- What do you think about using central Conda environment throughout? Current Augur docs specify environment.yml and I wanted to be sure this was an okay direction for you. I'm okay with either just closing this PR unmerged and keeping environment.yml throughout augur or going the PR route of nextstrain.yml. I learn towards the latter, especially as we have conflicting install instructions in the current setup that could lead to confusion.

@rneher
Copy link
Member

rneher commented Dec 15, 2019

moving to a single one definitely makes sense -- but I am getting increasingly confused were all the things are... I guess the only reason to keep this outside of the augur repo is the nodejs installation for augur. But I don't have strong feelings.

As you point out, there are a bunch of places in the docs that would need adjusting. When we previously taught nextstrain for Windows users, they often lack curl. So it needs to be explicit that they need to install it.

a few other things:

  • What part of nextstrain needs git?
  • cvxopt has been a finiky dependency that is only needed for titers. Do we want it to be part of the default install?
  • I believe python 3.7 works fine.

@huddlej
Copy link
Contributor

huddlej commented Mar 7, 2020

Getting to some of @rneher 's last points:

  • both curl and wget are available through conda's defaults channel, so if it would help with general use of Nextstrain (e.g., working through tutorials and workshops) we could add those
  • git used to be required by the old auspice export command and is now only referenced by developer scripts to build docs or revert releases. We should be able to remove it as a dependency
  • cvxopt installs from conda without any trouble and the latest version in PyPI (1.2.4) installs from pip without any issues. I've confirmed this version still works with the current titers code. We should keep this in the environment file, since it is required by a top-level augur command.
  • I've been testing Zika and flu builds with Python 3.7 while developing a new Docker base image and haven't had any issues

@huddlej
Copy link
Contributor

huddlej commented Apr 4, 2020

Making a note here that we need to update the installation docs to reference the conda repo YAML instead of the environment.yaml.

@trvrb
Copy link
Member Author

trvrb commented Jan 22, 2021

@kairstenfay just ran into this (https://bedfordlab.slack.com/archives/C0H7C3F0B/p1611191895001200). Can we just remove environment.yml from the Augur codebase in favor of https://github.com/nextstrain/conda? The conda repo was updated in Nov 2020, while environment.yml hasn't been updated since May 2020.

It looks like @huddlej fixed Travis to no longer use environment.yml in this commit: ff91b47

Is there anything else to be done here besides just deleting environment.yml?

@trvrb trvrb added easy problem Requires less work than most issues priority: moderate To be resolved after high priority issues labels Jan 22, 2021
@huddlej huddlej self-assigned this Jan 22, 2021
@huddlej
Copy link
Contributor

huddlej commented Jan 22, 2021

Thanks for bumping this, @trvrb! There are still a couple of references to environment.yml in the repo related to running tests before a release. These references can be replaced with simpler conda create commands like the Travis CI example.

I'm about to make a release this morning, so it would be simpler to make these changes after the release. I've assigned myself to this issue and moved it to the "Next up" pipeline.

@trvrb
Copy link
Member Author

trvrb commented Jan 22, 2021

Thanks John! Not urgent, but did want to bump given that this seemed close to complete.

huddlej added a commit that referenced this pull request Mar 18, 2021
Replaces references to local Conda environment file with conda install
commands that install the Augur package from Bioconda. This approach was
not previously possible before we started maintaining the Augur Bioconda
package.

As part of these changes, this commit also improves the usability of the
`./devel/test` script a bit.

Fixes #421
@huddlej huddlej closed this in #702 Mar 19, 2021
@victorlin victorlin deleted the update-conda branch June 30, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy problem Requires less work than most issues priority: moderate To be resolved after high priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants