-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Nextclade v3 alpha; remove Nextclade v1 #195
Conversation
I had to move v2 downloads from stage 2 (before CACHE_DATE) to stage 3 (after CACHE_DATE) because |
Looks like a good migration strategy to me, @victorlin is best placed to comment on the Dockerfile details. One challenge is that the symlinking strategy doesn't work for bioconda, unless we tweak the bioconda recipes to not just add This would be similar to Something like this should work for bioconda: bioconda/bioconda-recipes@master...corneliusroemer-patch-2, there's probably a better way including a link (soft or hard) to not duplicate the file, but I don't know enough conda for that. Apparently symlinking works: see https://github.com/conda-forge/python-feedstock/blob/07b5f66b338c611a60610283ac52b239038b1b3d/recipe/build_base.sh#L536 |
@corneliusroemer I don't see why we'd include multiple versions in the conda package. Conda packages are already versioned, so for sticking to v2 you'd just What I am talking about (on Slack) is that I cannot release If you mean that we can release conda package versioned Anyways, this PR is about |
@ivan-aksamentov Yes this is not important, but for context, the reason I brought up conda is that we try to keep nextstrain/docker-base and nextstrain/conda-base aligned so that users can switch seamlessly between runtimes. I don't suggest adding multiple versions into the conda package, just exporting an extra binary name, so that if we add IIUC, the point of adding v3 to this docker image is that we can use When we did the last upgrade there was no conda runtime to think about yet. ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem reasonable. I think eeaedcf can be reworked to leverage caching for v2 - I'll push a change.
@ivan-aksamentov said: It is obsolete and it is not adequate scientifically anymore, so if something happens to break missing v1 it should be fixed by upgrading.
@ivan-aksamentov said: This is the latest version with major 2 and there will be no more updates. The binaries are called nextclade2 and nextalign2. They are symlinked to nextclade and nextalign respectively, so that this version is still the default.
For readability.
@ivan-aksamentov said: We can start upgrading pipelines to v3. The downloaded version is currently hardcoded.
eeaedcf
to
81148f4
Compare
Thanks a lot @victorlin! Did you manage to resolve things? It says you're requesting changes but it's unclear which. Would be great to have that |
@corneliusroemer thanks for following up. I pushed the requested changes. Will run a quick test now on the preview image then merge when it looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Binary versions are as expected on PR branch's preview image:
$ docker run --rm -it docker.io/nextstrain/base:branch-nextclade-v3 nextclade3 --version nextclade 3.0.0-alpha.0 $ docker run --rm -it docker.io/nextstrain/base:branch-nextclade-v3 nextclade --version nextclade 2.14.0 $ docker run --rm -it docker.io/nextstrain/base:branch-nextclade-v3 nextalign --version nextalign 2.14.0 $ docker run --rm -it docker.io/nextstrain/base:branch-nextclade-v3 nextclade2 --version nextclade 2.14.0 $ docker run --rm -it docker.io/nextstrain/base:branch-nextclade-v3 nextalign2 --version nextalign 2.14.0
@corneliusroemer |
I already tested the image using the branch tag, nextclade worked as before, that is without 2 it was 2. So now we can transition to 3, as long as we use v3, but should add version checks in snakemake to warn conda users. |
Description of proposed changes
nextclade3
. We can start upgrading pipelines to v3. The downloaded version is currently hardcoded.nextclade2
andnextalign2
. They are symlinked tonextclade
andnextalign
respectively, so that this version is still the default.nextclade1
andnextalign1
) - it is obsolete and it is not adequate scientifically anymore, so if something happens to break missing v1 it should be fixed by upgrading.TODO after final v3 release:
nextclade
tonextclade3
, making it defaultnextalign
tonextclade3
, or, preferably, consider removingnextalign
symlink.Useful links:
Related issue(s)
Checklist
Checks pass
Binary versions are as expected on PR branch's preview image: