pull data from https instead of fauna #3
Conversation
Co-authored-by: Victor Lin <13424970+victorlin@users.noreply.github.com>
|
hmm, I might not have "merge" permissions for |
|
@j23414 I just added @nextstrain/core as a writer to this repo so you should be able to do so now. |
| - pip3 install git+https://github.com/nextstrain/cli | ||
| # https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/use-conda-with-travis-ci.html#the-travis-yml-file | ||
| - wget https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh -O miniconda.sh; | ||
| - bash miniconda.sh -b -p $HOME/miniconda | ||
| - source "$HOME/miniconda/etc/profile.d/conda.sh" | ||
| - hash -r | ||
| - conda config --set always_yes yes --set changeps1 no | ||
| - conda update -q conda | ||
| # Useful for debugging any issues with conda | ||
| - conda info -a | ||
| # Install nextstrain cli | ||
| - conda install -n base -c conda-forge mamba --yes | ||
| - conda activate base | ||
| - mamba create -n nextstrain -c conda-forge -c bioconda nextstrain-cli augur auspice nextalign snakemake git --yes | ||
| - conda activate nextstrain | ||
| - nextstrain version | ||
| - nextstrain check-setup | ||
| - nextstrain update |
There was a problem hiding this comment.
This diff seems like a step in the wrong direction to me. What was a single line turned into 14. :-/ There's also a lot of unnecessary stuff installed (since Docker is supported and what's used here), like augur, auspice, nextalign, etc. This feels like it was copy/pasted from elsewhere (where maybe it was all necessary, maybe not) without adjusting it for the new context, which is a recipe for degrading the codebase quality over time.
There was a problem hiding this comment.
From a "code smell" standpoint I agree with you @tsibley. The current expansion was a workaround until pip3 install was fixed for .travis testing.
See nextstrain/zika#14 for more details. Open other suggestions.
There was a problem hiding this comment.
Suggestions:
- Move to GitHub Actions where we're using
conda-incubator/setup-miniconda@v2in repos like augur - Only do
mamba install nextstrain-cli - Make it more clear that Docker is being used. Maybe
nextstrain check-setup --set-default? Unless there is a way to do something likenextstrain --set-runtime docker
There was a problem hiding this comment.
I like option 1, to move toward a consistent CI across repos :)
There was a problem hiding this comment.
Ah, I didn't see nextstrain/zika#14. That's useful context.
I don't care too much about specifically conda/mamba vs. pip and Travis CI vs. GitHub Actions here, but good to be consistent when it makes sense (e.g. prefer GitHub Actions over Travis CI) and trim things down to minimal necessary steps.
Specifically here, I would wonder why did pip install start failing? Does it work now (i.e. was it transient)? It should work, and it's the simplest step (can be installed from latest PyPI release, doesn't have to be git and probably shouldn't be).
There was a problem hiding this comment.
Make it more clear that Docker is being used
I think adding the --docker flag to the nextstrain build invocation would suffice ± a comment.
There was a problem hiding this comment.
Lol, I think we were both taxing .travis test at the same time.
As long as it's working now, I'm happy
We've been slowly moving everything off Travis CI in favor of centralizing on GitHub. More acutely motivated here by some Travis-specific errors, previously worked around by <#14>. Reverts back to a simple pip install instead of conda/mamba setup as much of it was unnecessary. See discussion in <nextstrain/measles#3 (comment)>.
We've been slowly moving everything off Travis CI in favor of centralizing on GitHub. More acutely motivated here by some Travis-specific errors, previously worked around by <#14>. Reverts back to a simple pip install instead of conda/mamba setup as much of it was unnecessary. See discussion in <nextstrain/measles#3 (comment)>.
Description of proposed changes
To match the zika workflow, pull data from https instead of fauna (rethinkdb).
Related issue(s)
Related to pull nextstrain/zika#15
Testing
To test the full pipeline:
To test the smaller test dataset