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

Pull before build if explicitly using :latest image? #166

Open
2 tasks
tsibley opened this issue Apr 1, 2022 · 2 comments
Open
2 tasks

Pull before build if explicitly using :latest image? #166

tsibley opened this issue Apr 1, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@tsibley
Copy link
Member

tsibley commented Apr 1, 2022

Consider always attempting to update the image before build (and maybe view, etc) if the user is explicitly using the :latest image (see #163) + also maybe other non-build-* image?

The benefit is that this really would make the --docker runner track the leading edge of development without still requiring frequent nextstrain updates. The downside is that the image used could change between two back-to-back runs, which might be surprising and unexpected.

By comparison, however, this would make build --docker closer to build --aws-batch which pulls by default before starting the job and thus will track the actual latest :latest image. We intentionally submit Batch jobs which use the same image by tag name (not by digest…) as is used locally in order to make the runtime more reproducible/predictable across the local/remote boundary. The difference in auto-pulling between --docker and --aws-batch makes this less predictable for the case of mutable tags like nextstrain/base:latest.

My current thinking is to:

  • Further nail down the local image == remote image by switching from specifying tag names in job specs to specifying image digests. This effectively renders moot the Batch auto-pull.

  • Re-introduce auto-pull as a CLI feature that acts the same across runners, but only apply it to non-build-* tags (since build-* tags aren't expected to be mutable).

    In terms of UX, I think it's probably best to make the auto-pull silent unless it results in an image update, in which case have it emit a one-line info message (e.g. "Updated runtime image from X → Y").

Net effect being auto-tracking of :latest as desired and higher consistency than currently between build --docker and build --aws-batch.

@tsibley tsibley added the enhancement New feature or request label Apr 1, 2022
@tsibley tsibley changed the title [docker] Pull before build if explicitly using :latest image? Pull before build if explicitly using :latest image? Apr 1, 2022
@victorlin victorlin moved this from New to Backlog in Nextstrain planning (archived) Apr 27, 2022
@tsibley
Copy link
Member Author

tsibley commented May 9, 2023

Re: using digests, see also nextstrain/docker-base#70 (comment).

@tsibley
Copy link
Member Author

tsibley commented May 25, 2023

Rough WIP patch for always pulling before use for Docker and Singularity runners:

diff --git a/nextstrain/cli/runner/docker.py b/nextstrain/cli/runner/docker.py
index 0ffe971..7a90b63 100644
--- a/nextstrain/cli/runner/docker.py
+++ b/nextstrain/cli/runner/docker.py
@@ -78,6 +78,8 @@ def run(opts, argv, working_volume = None, extra_env = {}, cpus: int = None, mem
     uid = getattr(os, "getuid", lambda: None)()
     gid = getattr(os, "getgid", lambda: None)()
 
+    _, explicit_tag = split_image_name(opts.image, implicit_latest = False)
+
     return exec_or_return([
         "docker", "run",
         "--rm",             # Remove the ephemeral container after exiting
@@ -126,6 +128,10 @@ def run(opts, argv, working_volume = None, extra_env = {}, cpus: int = None, mem
         *(["--memory=%db" % memory]
             if memory is not None else []),
 
+        # Always refresh an explicit "latest" tag
+        *(["--pull=always"]
+            if explicit_tag == "latest" else []),
+
         *opts.docker_args,
         opts.image,
         *argv,
diff --git a/nextstrain/cli/runner/singularity.py b/nextstrain/cli/runner/singularity.py
index 9e7a671..ad86396 100644
--- a/nextstrain/cli/runner/singularity.py
+++ b/nextstrain/cli/runner/singularity.py
@@ -84,7 +84,10 @@ def run(opts, argv, working_volume = None, extra_env = {}, cpus: int = None, mem
     #   -trs, 5 Jan 2023
     image = f"docker://{opts.image}" if not opts.image.startswith("docker://") else opts.image
 
-    if not image_exists(image):
+    _, explicit_tag = split_image_name(docker_image_name(image), implicit_latest = False)
+
+    # XXX FIXME: avoid redownloading/converting if it's not changed… similar to docker
+    if not image_exists(image) or explicit_tag == "latest":
         if not download_image(image):
             raise UserError(f"Unable to create local Singularity image for {image!r}.")
 
@@ -276,7 +279,7 @@ def download_image(image: str = DEFAULT_IMAGE) -> bool:
 
     try:
         subprocess.run(
-            ["singularity", "build", path, image],
+            ["singularity", "build", "--force", path, image],
             env   = env,
             check = True)
     except (OSError, subprocess.CalledProcessError):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Backlog
Development

No branches or pull requests

1 participant