Nextflow-compatible image, faster streaming reads, release 0.5.1 - #12
Merged
Merged
Conversation
Nextflow requires /bin/bash to be the container entrypoint, so ENTRYPOINT ["adata"] made every Docker- or Podman-backed process fail with `No such command '/bin/bash'` -- Nextflow invokes `docker run IMG /bin/bash -ue .command.sh`. Apptainer users were unaffected, since `singularity exec` ignores the entrypoint, which is probably why this went unnoticed. Drop the entrypoint and spell the command out in CMD instead. Nextflow also needs bash, ps, awk, date, grep, sed, tail and tee in the task container to collect metrics. procps is not in bookworm-slim, so every task silently lost its trace row. Install it, and assert the whole set at build time so base-image drift fails the build rather than every task. Two further fixes for bind-mounted runtimes: - PYTHONNOUSERSITE, because Apptainer bind-mounts the host $HOME and a user's ~/.local site-packages would otherwise shadow the venv. - XDG_CACHE_HOME, because Nextflow is commonly configured with `-u $(id -u):$(id -g)`, leaving no writable $HOME. Add the missing .dockerignore. Without one, `COPY . .` pulled the host's .venv, .git, .pytest_cache and .claude/worktrees (two full repo copies) into every local build; CI never hit this because a fresh checkout has none of them. Also move the apt and duckdb layers ahead of `COPY . .`, so a source edit no longer re-downloads duckdb. Verified against Nextflow 26.04.6: the pipeline completes and the trace is populated (%cpu=296.5%, peak_rss=11.2 MB). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A container-only release: the Python package is unchanged. Tagging it is what republishes the image, since .github/workflows/quay-on-tag.yml only builds on a tag push and the 0.5.0 tag must not be moved -- dropping the entrypoint is a breaking change for anyone pinned to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcd4d2479a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
_chunk_step returned the source's chunk height verbatim, so a store chunked (1, n_cols) was copied one row per read. On a local disk that is merely wasteful; on Lustre or NFS every read is a round-trip costing milliseconds, so a million-row copy spent nearly all of its time waiting. Reads are now grown to a 32 MiB budget and rounded down to a whole number of source chunks, since a partial read still decompresses the whole chunk. A (1_000_000, 30_000) float32 store chunked (1, 30_000) goes from 1 row per read to 279. Sizing needs the dtype, which h5py misreports for variable-length strings: itemsize is 8 there because the value is a pointer, not the text. Assume VLEN_ELEMENT_BYTES instead, so the row count is not overestimated by an order of magnitude and the memory bound holds. The step is floored at one whole chunk, which makes the budget a target rather than a cap for a source whose own chunk already exceeds it. That matches the previous behaviour and is now commented as such. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Setting XDG_CACHE_HOME was self-defeating as written: uv honours it, so `uv sync` created /tmp/.cache root-owned and mode 0755 during the build. A task running under `-u $(id -u):$(id -g)` then could not write to the very path the image advertises as its cache, which is worse than leaving the variable unset. Clear the directory and recreate it world-writable in the same layer as the sync. Verified: `mkdir $XDG_CACHE_HOME/probe` now succeeds as an arbitrary UID, where it failed with EACCES before. Reported by Codex review on #12. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two independent fixes, released together as 0.5.1.
1. The image could not be used from a Nextflow process
Three problems, one of them a hard failure:
ENTRYPOINT ["adata"]. Nextflow requires/bin/bashto be the container entrypoint — it invokesdocker run IMG /bin/bash -ue .command.sh, which the entrypoint mangled intoadata /bin/bash -ue .command.sh. Reproduced against the old behaviour:Apptainer users were accidentally fine, since
singularity execignores the entrypoint — likely why this went unnoticed. The olddocker.entrypointOverrideescape hatch is gone from the current Nextflow config reference.No
ps. Nextflow needsbash (>=3.0),ps,awk,date,grep,sed,tailandteein the task container to collect metrics.procpsis not inbookworm-slim, so every task silently lost its trace row.Bind-mounted runtimes. Apptainer bind-mounts the host
$HOME, so a user's~/.local/lib/python3.12/site-packagescould shadow the image's virtualenv; and Nextflow is commonly configured with-u $(id -u):$(id -g), which leaves no writable$HOME.What changed
Dockerfile— droppedENTRYPOINTforCMD ["adata", "--help"]; addedprocpsandmawk; keptcurl/unzip/ca-certificatesrather than purging them; setPYTHONNOUSERSITE=1,XDG_CACHE_HOMEandUV_COMPILE_BYTECODE=1. A build-timeRUNasserts the required tool set, so base-image drift fails the build rather than every task..dockerignore— new. There wasn't one, soCOPY . .pulled the host's.venv,.git,.pytest_cacheand.claude/worktrees/(two full repo copies) into every local build. CI never hit it because a fresh checkout has none of them. The apt and duckdb layers also moved ahead ofCOPY . ., so a source edit no longer re-downloads duckdb.Docs —
README.mdanddocs/index.mdboth documenteddocker run IMAGE view file.h5ad, which the entrypoint change makes wrong. Both now name the command.Breaking change
The image no longer sets an entrypoint, so the command must be named explicitly:
This is why it ships as 0.5.1 rather than by moving the
0.5.0tag: anyone pinned to0.5.0would otherwise break silently.2. Copying a row-chunked store was dominated by read latency
_chunk_stepreturned the source's chunk height verbatim, so a store chunked(1, n_cols)was copied one row per read. On a local disk that is merely wasteful; on Lustre or NFS every read is a round-trip costing milliseconds, so a million-row copy spent nearly all of its time waiting.Reads are now grown to a 32 MiB budget and rounded down to a whole number of source chunks, since a partial read still decompresses the whole chunk:
(1_000_000, 30_000)f32(1, 30_000)(10_000_000,)i64(65_536,)Sizing needs the dtype, which h5py misreports for variable-length strings —
itemsizeis 8 there because the value is a pointer, not the text.VLEN_ELEMENT_BYTESis assumed instead, so the row count is not overestimated by an order of magnitude.Known limit, deliberately kept: the step is floored at one whole chunk, so
TARGET_READ_BYTESis a target rather than a cap. A source whose own chunk already exceeds the budget — say(1000, 1_000_000)f32 chunked whole — reads 3.8 GiB in one go. That matches the previous behaviour exactly, so nothing regresses, and it is now commented at the call site rather than left as an implied guarantee._chunk_stepis private with a single production call site, updated in the same commit.Review
Codex flagged that
XDG_CACHE_HOMEwas self-defeating as first written —uv synchonours it, so the build left/tmp/.cacheroot-owned and mode 0755, and a task under an arbitrary UID could not write to the very path the image advertises. Correct, confirmed, and fixed in 9a77450 by clearing and recreating the directory1777in the same layer as the sync. Thread resolved.Verification
Built the image and ran a real pipeline under Nextflow 26.04.6 with
docker.runOptions = '-u $(id -u):$(id -g)'. It completes, and the trace is populated — direct evidence thatpsworks:Also confirmed on the final image: entrypoint is
[]; all eight required tools resolve;$XDG_CACHE_HOMEis writable as UID 12345 and 999; a deliberately poisoned~/.local/.../zarr.pyis ignored in favour of the venv; no.git/.claude/.pytest_cachein the image. Full suite: 1037 passed.Left out, deliberately
quay-on-tag.ymlstill builds amd64 only. Fine for the farm, but local Apple Silicon builds now diverge in architecture from the published image. A one-lineplatforms:addition fixes it if wanted.OMP_NUM_THREADSis not pinned in the image. NumPy's BLAS sizes its pool to the whole host, which oversubscribes a shared LSF node — but that belongs in a pipeline'senvscope, not baked in. Noted in a Dockerfile comment.Releasing
quay-on-tag.ymlonly builds on a tag push and has noworkflow_dispatch, so after merge:The tag must match
pyproject.tomlexactly orpublish.yml'scheck-versionjob fails. That push also publishes 0.5.1 to PyPI and moves thelatestimage tag.🤖 Generated with Claude Code