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-evaluate the way some Nextstrain components are added #129

Closed
victorlin opened this issue Feb 24, 2023 · 8 comments
Closed

Re-evaluate the way some Nextstrain components are added #129

victorlin opened this issue Feb 24, 2023 · 8 comments
Labels
proposal Proposals that warrant further discussion

Comments

@victorlin
Copy link
Member

This topic was mentioned in #44 (comment) and #128 (comment).

Current Behavior

nextstrain-cli and evofr are installed from PyPI using pip3 install <package>.

Nextclade and Nextalign pre-built binaries are downloaded from the latest GitHub Release in the Nextclade repo.

Fauna, Augur, and Auspice are installed from source.

Questions

Why are the last 3 installed from source?

This is what I can gather:

  • Fauna is not versioned nor published to any registry, so it must be installed from source.
  • Augur and Auspice's primary reason is mentioned in the comments: by doing so, the source can be replaced on runtime using something like --volume=.../augur:/nextstrain/augur.

Should Augur and Auspice be installed from PyPI/NPM instead?

This is where I'd like to hear more input. Installing from their respective package registries would remove the need to determine the latest version, and is more consistent with installation of other tools.

The trade-off is that installing from PyPI/NPM would remove the ability to replace the version upon runtime. That ability has been available for Augur from the start, and extended to Auspice in 0b27e44. I'm curious what the original motivation was for this and if that motivation still applies today.

@tsibley
Copy link
Member

tsibley commented Feb 27, 2023

The source-based installs of Augur and Auspice are what allow the --augur <path> and --auspice <path> options of Nextstrain CLI's Docker runner, e.g. as documented in nextstrain build --help-all or nextstrain shell --help-all

The intended usage scenario of those is during development or testing of a small fix or new feature in Augur or Auspice², when you want to give it a whirl inside the full, actual runtime environment instead of your local development environment. I've used those options very sporadically over the years, but my most recent use was still in the past month or two. That said, I don't know how much any others have used that functionality. If it poses a continued obstacle to other improvements, it's maybe time to ditch it.

¹ As an aside, this caused me to notice that --help became equivalent to --help-all in a regression on Python ≥3.10, so I fixed it.

² As long as it doesn't require new/updated deps.

@jameshadfield
Copy link
Member

jameshadfield commented Mar 7, 2023

Auspice just uses a trigger to rebuild the docker image - there's no determining the latest version, right?

docker-base/Dockerfile

Lines 251 to 252 in d59452c

RUN /builder-scripts/download-repo https://github.com/nextstrain/auspice release . \
&& npm update && npm install && npm run build && npm link

I don't know how much any others have used that functionality.

I haven't used this functionality in a long time, however I've been playing with docker recently and am looking forward to using it (docker) as part of my day-to-day work.

I suspect this is more useful for augur compared with auspice? For auspice, any code changes require the bundle to be rebuilt on the host machine as it's bind-mounted into the container which just uses the built (bundled) assets (AFAIK - (I avoid npm link where possible so I may be misunderstanding what's happening in our dockerfile). I'm interested in exploring how to rebuild the docker image using auspice source code copied in from the host rather than GitHub, as this would test building in the container environment as well as running it, but that's by-the-by 😉

@victorlin
Copy link
Member Author

@jameshadfield

Auspice just uses a trigger to rebuild the docker image - there's no determining the latest version, right?

Correct, the release branch of the Auspice repo contains tha latest version, so it is "determined" from there which is appropriate. The issue is more with Augur, where the latest version is currently determined from PyPI (half the reason for issue #128).

I'm interested in exploring how to rebuild the docker image using auspice source code copied in from the host rather than GitHub

I believe what you're looking for is the ability to use Auspice source code copied from the host into the pre-built Docker image (not rebuilding the image itself) - this is what's enabled by the --auspice <path> options @tsibley mentioned above.


It looks like there are still use cases for having Augur/Auspice installed from source. Of the 2 benefits I mentioned for installing from PyPI/NPM, I don't think either of them justify removing the source-install feature. There are other ways to improve the way the latest version is determined for Augur.

@jameshadfield
Copy link
Member

I'm interested in exploring how to rebuild the docker image using auspice source code copied in from the host rather than GitHub

I believe what you're looking for is the ability to use Auspice source code copied from the host into the pre-built Docker image (not rebuilding the image itself) - this is what's enabled by the --auspice options @tsibley mentioned above.

Not quite -- there's some subtlety here. The docker container uses the built (bundled) auspice code rather than the source code, so if you use --auspice <path> then you must remember to rebuild auspice on your machine in order for changes to be visible from the container (e.g. npm run build from your machine after any changes to source code), and so you are testing auspice built using your environment not built from the docker environment. For augur I see a bigger advantage to bind-mounting -- there's not the equivalent build step and augur interacts with a bunch of other tools while it runs (auspice is more stand-alone). I could accomplish what I was musing about via docker exec -it <container_id> bash and rebuild auspice from within the running container, or by modifying the dockerfile and rebuilding the image, but neither may be worth it!

@tsibley
Copy link
Member

tsibley commented Mar 10, 2023

…and so you are testing auspice built using your environment not built from the docker environment.

Yeah, the use case for --auspice was always "run a modified Auspice inside the Docker environment", not "build a modified Auspice…". I've used it for testing of nextstrain view's interaction with a modified Auspice, for example.

Do you have a need to test Auspice built using the Docker environment?

@corneliusroemer
Copy link
Member

I've briefly looked at why docker build actions take so long (~1hr) and it seems that the biggest single time sink is Auspice build. Could we reduce the build time by moving the Auspice build step earlier so that it only needs to build when we have a new Auspice release? It seems to be the most time-intensive build step, accounting for at least half of the build time of ~1hr.

Does anyone actually use that --auspice option with docker? What's the use case? To make development work easier? Why don't we have a dedicated development container for that purpose?

@tsibley
Copy link
Member

tsibley commented May 8, 2023

@corneliusroemer

Does anyone actually use that --auspice option with docker? What's the use case? To make development work easier? Why don't we have a dedicated development container for that purpose?

… did you read either of my comments above? I find it hard to see how you would have these questions if you had.

@victorlin victorlin added the proposal Proposals that warrant further discussion label May 14, 2024
@victorlin
Copy link
Member Author

Coming back to this after a while. Two motivations have been addressed:

  1. Remove the need to determine latest version – this doesn't have any added benefit as discussed in Different Augur versions between amd64 and arm64 images #128.
  2. Remove the need for a long-running Auspice build – this has been improved with cross-compilation+caching.

Closing with the decision that things should be kept as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposals that warrant further discussion
Projects
No open projects
Development

No branches or pull requests

4 participants