XNAT dcm2niix conversion pinned to stale 2021 Docker Hub build silently drops slices from valid series - #981
Conversation
…HCR image
Docker Hub's mutable xnat/dcm2niix:latest resolves to dcm2niix v1.0.20211006 —
a 2021 build upstream no longer updates — whose slice-ordering heuristic
mis-fires on some of the seeded spleen series ("Slices skipped: image position
not sequential" on clean, uniformly-spaced 0-based-InstanceNumber series) and
silently drops one slice. The converted input_*.nii.gz then has N-1 slices
while the enrichment label has N, so the segmentation tutorials' shape QC
rejects 6 of GSTT's 21 spleen studies. The current upstream release
(v1.0.20260724) converts the same series in full.
- New trust/xnat/dcm2niix/Dockerfile: debian-slim + the upstream release
binary, pinned by version AND sha256, with a build-time version smoke check.
- New docker_build_xnat_dcm2niix.yml workflow publishing
ghcr.io/londonaicentre/xnat-dcm2niix with the usual sha/branch/stag/prod
tags plus the tool-version tag (read from the Dockerfile ARG) that all
deploy configs reference — also resolves the PR #945 review note about the
converter still coming from Docker Hub.
- dcm2niix_command.json (Compose), the K8s init-job's inline copy, and
imaging-api (new Settings.DCM2NIIX_IMAGE, used by the per-project event
subscription lookup) now reference the immutable version tag; command
version bumped 1.6 -> 1.7.
- Verified: the built image converts the previously slice-dropping series
(FAK76374103) 60/60 and a control series 92/92 with the Container Service's
exact command line; imaging-api ruff+mypy+274 tests and trust/xnat's 174
tests pass.
Closes #980
Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… swarm launches don't NPE Found by an end-to-end pathway test with a dummy GHCR tag: with the dcm2niix image moved off Docker Hub, every swarm-mode Container Service launch died with 'NullPointerException: authConfig was not specified' before a service existed. container-service 3.8.1 resolves the launch's registry auth by matching an image-host url against the bare registry hostname parsed from the image string, and the swarm path (unlike the pull path) passes a null resolution to docker-java unguarded - so a registry with no image-host entry is unlaunchable even when its image is public and needs no credentials. configure-dcm2niix.sh now registers the pinned image's registry as a credential-less image host (hostname derived from dcm2niix_command.json so it cannot drift; idempotent; validated after the fact). The Kubernetes backend needs no entry - kubelet does the pulling there - which the init job now documents. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
0eb0b1d to
9fa0553
Compare
garciadias
left a comment
There was a problem hiding this comment.
Re: deploy/README.md (line 269) — flagging at PR level because this line is unchanged context just outside the diff hunks.
This section states "All Docker containers in FLIP are hardened" and the Non-Root Users table enumerates every service, but the new xnat-dcm2niix image introduced in this PR deliberately keeps the root default user (see trust/xnat/dcm2niix/Dockerfile), and it is missing from the table. The blanket claim is now inaccurate and the hardening doc is incomplete for exactly the image being added.
Suggested change: add a xnat-dcm2niix | root | Container Service one-shot; keeps the root default user to match the replaced image's output-file ownership on the build mount row, and adjust the intro sentence to acknowledge this intentional exception.
garciadias
left a comment
There was a problem hiding this comment.
Correct pin of the dcm2niix image, but one operational gap blocks merge: the GHCR package visibility flip is a manual post-merge step recorded only in the PR body, so the first publish will produce a private image that deployments cannot pull — the exact silent failure this fixes. Please record it in committed docs (and ideally add an unauthenticated manifest probe).
The GHCR package-visibility flip lived only in the PR description, which does not survive the merge. New org packages default to private and GITHUB_TOKEN cannot change that, while the XNAT Container Service pulls the converter anonymously — so a private package registers fine at deploy time and only fails when a scan is archived at a trust. Record the operator step in trust/xnat/README.md (pointed at from deploy/README.md), and make forgetting it loud: the publish workflow now ends with a credential-less manifest probe of the tag it just pushed, so the run stays red until the package is public. The image string is also pinned literally in three deploy configs beside the Dockerfile ARG that decides the published tag, and a bump that misses one is silent (the old immutable tag keeps being pulled). Add trust/xnat/dcm2niix/check_image_pin_sync.sh, wired as a pre-commit hook and as a dcm2niix-pin-sync CI job in test_trust_xnat.yml. Also drop the dead workflow_run checkout inputs, GH_WR_* env and BRANCH_NAME arm from the new workflow: it is triggered by push and workflow_dispatch only, so those expressions always evaluated empty and only worked through actions/checkout fallback behaviour. Document the deliberately root-running xnat-dcm2niix image in deploy/README.md Non-Root Users. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
|
On the PR-level point about the "Non-Root Users" table in The row records both halves of why it is an exception rather than an oversight:
Also worth flagging for the merge itself, since it came up in the visibility thread: |
garciadias
left a comment
There was a problem hiding this comment.
Verified the fix is complete: grepping the full PR-head tree confirms no leftover xnat/dcm2niix:latest references anywhere in code, config, tests, or docs, and all four pin sites (Dockerfile ARG, dcm2niix_command.json, K8s init-job, imaging_api/config.py) agree on v1.0.20260724, enforced in sync by the new check_image_pin_sync.sh. Manual verification (re-converting the previously slice-dropping accession 60/60, a control series 92/92, plus a full live platform pathway test) is concrete and convincing.
Left two comments on the new publish workflow: it has no pull_request trigger, so a broken Dockerfile/checksum on a future version bump would only surface after merging to develop -- the same "nobody notices" pattern this PR fixes for the pin itself -- plus a small dead-code leftover in the tag-computation step.
A wrong DCM2NIIX_SHA256 or a broken download/smoke chain previously failed only after merging to develop — the deferred-failure pattern this PR exists to remove. PR runs now exercise the full docker build (pinned download, sha256 check, baked-in smoke test); the GHCR login, push and anonymous-pull-visibility steps are gated on the event name so nothing is published from a pull request. Same pattern as docker_build_orthanc (FLIP-PT-091). This also makes the previously unreachable pr-<n> tag block in the Determine tags step live. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
Closes #980.
Problem
The XNAT Container Service converts DICOM→NIfTI with Docker Hub's mutable
xnat/dcm2niix:latest, which resolves to dcm2niix v1.0.20211006 — a 2021 build upstream no longer updates (no newer tag exists on that repo). Its slice-ordering heuristic mis-fires on some of the seeded spleen series and silently drops one slice:The series are provably clean (unique 0-based InstanceNumbers, uniform 5 mm ImagePositionPatient steps, constant orientation; complete at every hop — Orthanc 60/60, XNAT DICOM resource 60/60). The converted
input_*.nii.gzthen has N−1 slices while the data-enrichment label has N, so the segmentation tutorials' shape QC (correctly) rejects the pair — 6 of GSTT's 21 spleen studies are silently lost from every spleen cohort, and always have been; the QC skips at INFO level so nobody noticed.Fix
Build and publish a version-pinned in-house image, and reference it by immutable version tag everywhere (never
latest):trust/xnat/dcm2niix/Dockerfile— debian-slim + the upstream release binary, pinned by version and sha256, with a build-time version smoke check. The Dockerfile'sDCM2NIIX_VERSIONARG is the single source of truth for the pin..github/workflows/docker_build_xnat_dcm2niix.yml— publishesghcr.io/londonaicentre/xnat-dcm2niixwith the usual sha/branch/stag/prod tags plus the tool-version tag read from the Dockerfile. (GHCR images in this org are anonymously pullable, so the Container Service's pull path needs no auth plumbing.) Also resolves the PR [Feature]: Add MONAILabel to OHIF viewer plugin XNAT #945 review note about the converter still coming from Docker Hub.org.opencontainers.image.source, which links the published package to this repo so it is listed under FLIP/packages alongside the other FLIP images. The siblings got that link implicitly (their packages were first created by aGITHUB_TOKENpush from this repo's workflows); this package already exists from the pathway test's manual push, so it needs the explicit label.trust/xnat/xnat/config/dcm2niix_command.json(Compose deploy; command version bumped 1.6 → 1.7)deploy/providers/kubernetes/templates/xnat-init-job.yaml(the documented inline copy of the command)trust/imaging-api— newSettings.DCM2NIIX_IMAGEused by the per-project event-subscription lookup (which finds the XNAT command by image string)trust/xnat/xnat/config/configure-dcm2niix.sh— additionally registers the pinned image's registry (ghcr.io, bare hostname, derived from the command JSON so it can't drift) as a credential-less Container Service image host, idempotently, with post-hoc validation. Load-bearing on the swarm backend — see the pathway-test finding below. The K8s init job deliberately doesn't (kubelet pulls there), and now documents why.trust/xnat/README.md,deploy/README.md,component-xnat.rst.Verification
dcm2niix -b n -z y -o /output /input) against the live GSTT archive data. No warnings.helm templaterenders the chart with the new image ref.test-e2etag (package made public — the CS pulls with no credentials), registered a parallel1.7-testcommand on the dev XNAT (leaving the live command and its subscriptions untouched), ran the PR-branch imaging-api withDCM2NIIX_IMAGEpointed at the test tag, and drove a fresh platform project through cohort → approval → image pull:CommandActionProvider:2), old projects untouched.NullPointerException: authConfig was not specified— container-service 3.8.1 resolves the launch's registry auth by matching an image-hosturlagainst the bare registry hostname parsed from the image, and the swarm path passes a null resolution to docker-java unguarded. A registry with no image-host entry is unlaunchable even for a public image. Hence theconfigure-dcm2niix.shimage-host registration above; without the pathway test this would have shipped broken for every swarm deployment.test-e2efrom GHCR (no local copy existed — genuine registry pull), the conversion completed (container [Task]: Upgrade Python 3.12 → 3.13 across all services (and record why 3.14 is blocked) #944), and the previously slice-dropping accession FAK76374103 produced the full (512, 512, 60) NIfTI through the real event/launch machinery.test-e2etag and the package it created have since been deleted. That manual push created the package outside Actions, which left it unlinked from this repo (repository: nullin the org packages API) — deleting it means the first post-merge publish recreates it from the workflow, labelled and linked, exactly as every other FLIP package came to be. See the post-merge checklist below.Deployment notes
workflow_dispatchof the new workflow is impossible until it exists on the default branch, so the first merge to develop publishes the image (the push event runs the workflow from the merged tree). Existing deployments pick the new image up on their next XNAT (re)configure — running stacks keep the old registered command until then.Post-merge checklist (the package no longer exists, so the first publish creates it from scratch):
gh api /orgs/londonaicentre/packages/container/xnat-dcm2niix --jq .repository.full_name→londonaicentre/FLIP(it should now be listed under the repo's Packages).