-
Notifications
You must be signed in to change notification settings - Fork 374
AN-736 Add support for container
attribute in WDL 1.1
#7809
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
base: develop
Are you sure you want to change the base?
Conversation
* Trait to handle validation of both 'docker' and 'container' runtime attributes, which are mutually exclusive | ||
* ways of specifying the container image to use for a task. | ||
*/ | ||
trait ContainersValidation extends OptionalRuntimeAttributesValidation[Containers] { |
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.
This needs to be optional because docker
and container
are each optional.
val eventPattern = | ||
"Key/s [test] is/are not supported by backend. Unsupported attributes will not be part of job executions." | ||
EventFilter.warning(pattern = escapePattern(eventPattern), occurrences = 1) intercept { | ||
EventFilter.info(pattern = escapePattern(eventPattern), occurrences = 1) intercept { |
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.
Drive-by fix of something that's been annoying me for years.
} | ||
} | ||
} | ||
// TODO |
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.
To discuss: This test fails now because the lack of a container or docker attr isn't detected at this stage of the validation process. It's detected when we actually try to get the container from the attrs via Containers.extractContainer
. I did some work (not checked in yet) to optionally include a "must include one of these" check in the Containers.validate
function, but that's not actually used in backend initialization - only validateValue
is used. See BackendWorkflowInitializationActor.validateRuntimeAttributes
(link)
} | ||
} | ||
} | ||
// TODO |
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.
See comment on GcpBatchInitializationActorSpec
@@ -1,80 +1,36 @@ | |||
package wdl.transforms.biscayne | |||
|
|||
import cats.implicits.catsSyntaxValidatedId |
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 to this file and the below one are largely a revert of changes in #7389
final case class TaskDefinitionElementToWomInputs(taskDefinitionElement: TaskDefinitionElement, | ||
typeAliases: Map[String, WomType] | ||
) | ||
|
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.
Continued revert of #7389
override protected def validateValue: PartialFunction[WomValue, ErrorOr[String]] = { case WomString(value) => | ||
value.validNel | ||
} | ||
class ContainerValidation extends ContainersValidation { |
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.
Make these not the same name
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.
Looking good! Holding off on a full thumb as it looks like there are still a few things to patch up
* ECR [pull-through caches](https://docs.aws.amazon.com/AmazonECR/latest/userguide/pull-through-cache.html) can now be used to access Docker images. See [ReadTheDocs](https://cromwell.readthedocs.io/en/develop/backends/AWSBatch/) for details. | ||
|
||
### Progress toward WDL 1.1 Support | ||
* WDL 1.1 support is in progress. Users that would like to try out the current partial support can do so by using WDL version `development-1.1`. As of Cromwell 91, `development-1.1` includes: |
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.
Maybe clarify that this is not the first 1.1 feature?
* WDL 1.1 support is in progress. Users that would like to try out the current partial support can do so by using WDL version `development-1.1`. As of Cromwell 91, `development-1.1` includes: | |
* WDL 1.1 support is in progress. Users that would like to try out the current partial support can do so by using WDL version `development-1.1`. As of Cromwell 91, `development-1.1` adds support for: |
|
||
### Other changes | ||
* Removed unused code related to Azure cloud services. | ||
* Changed log level from WARN to INFO for messages about unsupported runtime attributes. |
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.
YES
|
||
val runtimeAttrKeys = List(RuntimeAttributesKeys.ContainerKey, RuntimeAttributesKeys.DockerKey) | ||
|
||
def apply(value: String): Containers = Containers(List(value)) |
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.
Elegant
case _ => None | ||
} | ||
|
||
// TODO enhance to select the best container from the list if multiple are provided. |
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.
It would be nice to link the relevant story here, I found its number: AN-734
@@ -0,0 +1,11 @@ | |||
name: container_attr_wdl10 |
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.
As a reminder, the search for *.test
cases is now recursive, so you can safely locate this file at standardTestCases/container_attr/container_attr_wdl10.test
Description
Add support for
container
as a preferred way to specify the image to run a task in in WDL 1.1. Also:docker://
prefix for images (Cromwell only supports Docker images currently)docker
orcontainer
to be a list of images. For now, always choose the first one.This ended up being very tricky because our runtime attribute validation system really, really wants each validation to be associated with a single key name and non-interacting with validation of other keys; and this validation system is pretty tightly coupled to call caching, metadata creation, and other things. I have many larger refactors that I got 75% of the way through implementing before giving up because they spiraled out of control. The current state of this code is technically not in-spec because it allows both
container
anddocker
to be defined for 1.1 WDLs (container is preferred if both are present).Release Notes Confirmation
CHANGELOG.md
CHANGELOG.md
in this PRCHANGELOG.md
because it doesn't impact community usersTerra Release Notes