Multiple container tags

[adthrasher]

Often multiple tasks with a file will utilize the same base tool (e.g. samtools) and therefore they will use the same container image. I would like to have a rule that warns for each container specification in the file if more than one tag is present in the file.

For example, the following set of tasks would yield a warning.

task merge {
   ...
  runtime {
     container: 'quay.io/biocontainers/samtools:1.17--h00cdaf9_0'
  }
}

task index {
   ...
   runtime {
      container: 'quay.io/biocontainers/samtools:1.20--h50ea8bc_0'
   }
}

Something like Container image quay.io/biocontainers/samtools has multiple tags.

@a-frantz and I have discussed this offline and would like a wider set of opinions.

EDIT:

Based on the conversations below, this lint rule will be restricted to only examining container entries with literal entries and exclude any set the value with a variable.

[a-frantz]

Many WDLs are structured so that the container value is a variable passed as an input. Would this rule simply ignore those cases? I think we could only evaluate literal values, correct?

[adthrasher]

That's a good question. I think (a controversial) related question is "Is allowing container to be a runtime variable a good choice?". I can see an argument for allowing a subset of it to be variable, for example the registry. But should a task specification allow any image? For example, I'd allow this style (https://github.com/PacificBiosciences/wdl-common/blob/fef058b879d04c15c3da2626b320afdd8ace6c2e/wdl/tasks/bcftools_stats.wdl) from PacBio where the registry is variable, but the container image and digest are fixed.

[yunhailuo]

I think allowing registry is definitely useful.

We've also seen a use case where only container gets updated and we have scientists want to jump back and forth with potential comparison. VEP is one of our example. When ensembl makes a new release, VEP has a new accompany release. Quite often the tool syntax doesn't change so to a compute job it's only data change. However, we also do a container upgrade to align with the version. So what end up happening is we either make a new release of WDL every single time or make the tag configurable.

I have to say I understand we might miss something and this might be a bad software practice. But naively, when a WDL task is a pure thin wrapper around a tool and the only change is the tool's own version and multiple versions are in active use (which could be bad), I feel I have a strong desire (good or bad) to just make tag configurable and simplify some devops work. Welcome comment either way.

[a-frantz]

@adthrasher What's the functional difference between using a runtime variable and overriding it at runtime with the JSON input? e.g. {"<workflow>.<task>.runtime.container": "foo"}

As long as the specification is permitting users to override runtime values at runtime, it's hard for me to accept that there's much more danger in permitting execution-time-evaluated-expressions.

However, I think there could be a case for returning to a situation where the runtime fields are "locked down" so-to-speak. By that I mean disallowing runtime overrides. Then you can explore lints where only certain expressions are permitted. But IMO, as long as a user can freely supply junk at runtime, I'm not seeing the point of this "runtime variable lint".

I stand by the premise of my first question: I think the OP lint is good, but needs to ignore non-literal cases. Whether we introduce lints for expression-based values is a separate discussion from my vantage-point.

[yunhailuo]

but needs to ignore non-literal cases

I want to second this. When people start to config, why they config could diverse a lot. And sometimes, I feel a missing feature is better and not annoying than a false positive feature. Also, starting small MVP and improving with iterations seem like a good practice to me.

[a-frantz]

@yunhailuo thanks! Me and @adthrasher touched base on this earlier offline, and a design philosophy we have for these lint rules is that they should be specific, narrowly-scoped, and easy to communicate. Which is to say I'm in favor of dividing and conquering with our rules. A lint rule specific to cases where there's a literal string as the container value should have one rule, and then there can be another rule for expression-values. We are going to run into trouble if a rule tries to "do too much".

[rickymagner]

I like the idea of this lint overall, but agree with the follow-up question that it might be tricky to implement when WDL variables are involved. For example, at Broad we have many pipelines using GATK docker images using the syntax:

String gatk_tag = "4.5.0.0"

...

docker: "<gatk_docker_hardcoded_path>:" + gatk_tag

I think a good implementation of this lint would have to contend with the case where the docker parameter is computed at execution time.

[rickymagner]

Can't speak for all use cases since I've seen a lot of them, but I think it's more to allow for easily updating the version without rewriting the WDL. Developers can bump the version as an input and test things work as expected/improve, and then recommend their users to update the input tag once it's been checked. It does help add consistency as well to ensure all images called in the workflow use the same tag. I can't imagine a use case where you really want to switch the version for just one task.

[adthrasher]

Does cromwell support overriding runtime attributes? I would think that doing something like "wf.t1.runtime.container": "gatk:4.5.0.0" as a runtime input would be a cleaner way to develop and test while leaving the version fixed for production usage.

[rickymagner]

I'm not sure, and if it does, it's probably not exposed via Terra, which is the use case I'm thinking of. Sometimes people have to do development in Terra for various reasons, so exposing the tag in this case is helpful.

[a-frantz]

My two cents, as expressed in the other thread, is that the lint rules should divide and conquer. We've got a specific, narrowly-scoped, easy to understand lint rule when we restrict the problem-set to only literal string values. Trying to handle values with execution-time resolution should be Out Of Scope for this rule.

@rickymagner if there's a lint rule you can think of that would help out your team in this context, feel free to start a new discussion for it! As our repo prefers using literal values, I feel unqualified to contribute a good rule to handle yall's case.

[rickymagner]

That sounds like a good idea. There are plenty of literal dockers used in some of our workflows, but it'd be great if this lint could gloss over the cases where they get evaluated at runtime.

[yunhailuo]

I'm also wondering how this gets play with this spec where

The container key also accepts an array of URI strings. All of the locations must point to images that are equivalent, i.e. they must always produce the same final results when the task is run with the same inputs. It is the responsibility of the execution engine to define the specific image sources it supports, and to determine which image is the "best" one to use at runtime.

?

Though it's not yet clear how engine should handle such things chanzuckerberg/miniwdl#683

[a-frantz]

That's pretty tricky @yunhailuo - Thanks for bringing this up, I was overlooking it!

My initial thoughts here, but I'd love to see some other opinions, is that this is another case that this proposed lint rule should ignore. My reasoning is simply that I can't think of any proposal that makes sense.

If there are two docker:// URIs, my understanding is that both would have to have the same digest to be considered equivalent. That requires information not necessarily contained in the WDL file, which makes me think it's OOS for sprocket.
And what if one URI points to a docker image and another points to a SIF image? How do you compare those to ensure equivalency? I don't know 🤷

This is a complicated case, and if anyone wants to pursue this, I would suggest it as a new rule (and new discussion) separate to this one.

[adthrasher]

That's a good point and a part of the spec that I'd forgotten. I will be curious to see how the engines intend to implement this section of the spec.