fix(aws-healthomics): reconcile WDL migration Phase 1 with the ECR pull-through cache SOP - #178
Merged
nadetastic merged 1 commit intoAug 6, 2026
Conversation
…he ECR SOP migration-guide-for-wdl.md Phase 1 required "Zero references to external registries remain" as a completion criterion, but ecr-pull-through-cache.md states that migrating workflows SHOULD use a container registry map (preferred) precisely so container URIs do NOT change. A map leaves the public URIs in the WDL, so the migration SOP defined the preferred approach's end state as failure. Phase 1 also prescribed hand-written docker pull/push scripts without referencing the eight container MCP tools documented in the same Power. Make the approach an explicit per-container choice, route staging through the MCP tools, link the ECR SOP, and split the Done WHEN criteria so each approach has its own. Add a transport-vs-identity decision rule to Technical Patterns: a map is right where the same repository and tag is served from private ECR, and the WDL must be edited where the repository changes, since the command block's correctness then depends on which image is pulled. Show both outcomes in the Container Runtime pattern, which previously showed only the URI-replacement result. Also replace "At least 5 key containers verified accessible from ECR" with per-task command coverage. Accessibility is not containment: an image can exist, pass describe-images, miniwdl check and CreateWorkflow, and still lack a tool the command block pipes to, which surfaces only as "command not found" once the task runs.
nadetastic
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reconciles the WDL migration guide's Phase 1 with the ECR Pull Through Cache SOP in the same Power. Today the two files prescribe opposite outcomes, and Phase 1 does not reference the other SOP or any of the eight container MCP tools it documents.
Also strengthens the container verification criterion from accessible to contains the tools the task invokes, because accessibility is not the property that makes a task run.
Only
migration-guide-for-wdl.mdis touched. The ECR SOP is unchanged — it is the file I believe is correct.1. The two files require opposite end states
ecr-pull-through-cache.md:15:ecr-pull-through-cache.md:135:But
migration-guide-for-wdl.md:50makes this a completion criterion for migration:Using a registry map means
quay.io/...stays in the WDL — that is the entire point of the map. The migration SOP's Done WHEN therefore defines the preferred approach's end state as failure. An agent that satisfies Phase 1 as written cannot have used a registry map, and an agent that follows the ECR SOP cannot satisfy Phase 1.Phase 1 also instructs the agent to author
scripts/migrate_containers_to_ecr.shdoing manualdocker pull/ tag /docker push(:35-39), while the same Power documentsCreatePullThroughCacheForHealthOmics,CloneContainerToECR,CreateContainerRegistryMap,CheckContainerAvailability, and four more (ecr-pull-through-cache.md:24-33). Phase 1 mentions none of them and does not link to that SOP.Change — Phase 1 step 3 makes the approach an explicit per-container choice, step 4 routes staging through the MCP tools and links the ECR SOP, and the Done WHEN bullets are split so each approach has its own criterion.
Zero references to external registries remainis preserved, scoped to the URI-replacement case.The
Container Runtime (Before/After)pattern showed only the URI-replacement outcome, which would have contradicted the new preference, so it now shows both — the registry-map "After" leavesdocker:unchanged and adds onlycpu/memory.2. Which approach applies when
Rather than declaring one approach correct, I added a decision rule to
## Technical Patterns— transport vs. identity. It is what makes both files right about different cases:bwawhile a map silently redirects it elsewhere runs something other than what the definition says, and that is invisible in the workflow definition.This distinction is also measurable, and one run shows both halves at once. A workflow with mixed references was registered without
--container-registry-map:Three things worth noting about that shape, all now stated in step 4:
CreateWorkflowreturnedACTIVE. The registry map is a workflow attribute supplied at creation, and registration did not validate the definition against it. A workflow that names public registries registers cleanly and is unrunnable. (I've worded this as "does not appear to validate" in the steering text, since this is one observation rather than documented behavior.)bwaran to completion and wrote its BAM — then the run died on the next task's URI. Everything before it was billed.samtools, and nothing is wrong with the samtools reference. It is a valid public quay.io URI. What is missing is a workflow-level attribute unrelated to samtools.3. Accessible is not the same as contains
workflow-development.md:60states the actual requirement:No procedure in the Power checks it.
migration-guide-for-wdl.md:51offers instead:Accessibility is a different property, and "at least 5" is an arbitrary sample of an inventory the same phase just enumerated in full.
Measured —
quay.io/biocontainers/bwa:0.7.18--he4a0461_0:The task using it pipes
bwa mem | samtools view -bS, so it dies at the pipe withsamtools: command not found. Everything upstream passed honestly:aws ecr describe-imagesOK,miniwdl checkOK,CreateWorkflowACTIVE. The image genuinely exists — that is why nothing objects.CheckContainerAvailabilitycannot see this either; it reports availability, as named.The only check that answers the question is running the image:
Swapping to a multi-tool
mulled-v2-*image fixes it, and that is a different repository — so by the rule in §2 it belongs in the WDL, not in a map. (In our case that also shifted samtools 1.21 → 1.20, which is the kind of thing worth recording in the inventory.)Change — the Done WHEN bullet becomes per-task command coverage rather than a count of accessible images. Step 5 gives the
docker runcheck, a heuristic for deriving the binary list from thecommandblock (so the criterion is executable rather than aspirational), what to do when a binary is absent, and why no static gate substitutes for it. Step 6 covers the multi-tool image case.Scope
migration-guide-for-wdl.mdonly. Not changed:ecr-pull-through-cache.md, the Nextflow migration guide (the same Phase 1 pattern likely applies there, but I have not measured it), and resource limit values.No file overlap with #175. That PR covers a different axis — registries that pull-through cache does not support, staged via
Registry Staging Mapping— whereas this one is about choosing between a registry map and URI replacement for PTC-supported registries. If #175 merges first I'm happy to add a pointer from Phase 1 step 4 toimage-staging.mdfor the non-PTC case, or to rebase in whichever order suits you.Measurement conditions
2026-07,
ap-northeast-2, two accounts (one with no pre-existing HealthOmics resources), miniwdl 1.15.0 / Python 3.12.13. AWS account IDs are masked as123456789012where they appeared in output; run and workflow IDs are left as-is since they are meaningless without the account.