Skip to content

Commit cb38c7c

Browse files
ClaptarClaptar
authored
Merge pull request #14 from cellgeni/feat/performance-testing
Fix the concat --merge hang, and close the gap that hid it
2 parents 7f4df63 + e8c24fb commit cb38c7c

31 files changed

Lines changed: 4632 additions & 67 deletions

‎.dockerignore‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# Keep the build context to what `uv sync --locked` actually needs. In
2+
# particular .venv/ would otherwise ship a host-platform virtualenv into the
3+
# image, and .claude/worktrees/ holds full copies of this repo.
4+
#
5+
# Do NOT add README.md or LICENSE here: pyproject.toml references them via
6+
# `readme` and `license-files`, so the build fails without them.
7+
.venv/
8+
.git/
9+
.claude/
10+
.pytest_cache/
11+
__pycache__/
12+
*.py[cod]
13+
.coverage
14+
coverage.xml
15+
htmlcov/
16+
build/
17+
dist/
18+
*.egg-info/

‎.github/workflows/benchmark.yml‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# Comparative benchmark: adata-cli against anndata and scanpy.
2+
#
3+
# Report-only, by design. Wall time on a shared runner is too noisy to gate a
4+
# release on, and a benchmark that can block a publish stops being run. What
5+
# *does* gate merges is tests/test_performance.py, which counts operations
6+
# rather than seconds and lives in the ordinary test job.
7+
#
8+
# Its own file rather than a job in publish.yml: at the `ci` tier this takes
9+
# the better part of an hour, and hanging that off the release graph would
10+
# either delay the PyPI publish or paint the release run red for a report.
11+
name: Benchmark
12+
13+
on:
14+
push:
15+
tags: ["*"]
16+
workflow_dispatch:
17+
inputs:
18+
tier:
19+
description: Input size
20+
type: choice
21+
options: [smoke, ci, large]
22+
default: ci
23+
publish:
24+
description: Commit the results to docs/ on main
25+
type: boolean
26+
default: false
27+
28+
concurrency:
29+
group: benchmark-${{ github.ref }}
30+
cancel-in-progress: true
31+
32+
jobs:
33+
benchmark:
34+
name: adata-cli vs anndata/scanpy
35+
runs-on: ubuntu-latest
36+
timeout-minutes: 90
37+
# Job level, not step level: a report must never mark a release red.
38+
continue-on-error: true
39+
permissions:
40+
contents: write
41+
steps:
42+
- uses: actions/checkout@v4
43+
with:
44+
fetch-depth: 0
45+
46+
- uses: astral-sh/setup-uv@v3
47+
with:
48+
enable-cache: true
49+
50+
# The checked-out source, not the published wheel. Benchmarking the
51+
# wheel would mean waiting on publish-pypi and then on index
52+
# propagation, for a measurement that would come out the same.
53+
- name: Install adata-cli
54+
run: uv sync --extra dev --frozen
55+
56+
- name: Show free space
57+
run: df -h /
58+
59+
- name: Run
60+
run: |
61+
uv run python -m benchmarks.run \
62+
--tier "${{ inputs.tier || 'ci' }}" \
63+
--out results.json \
64+
--work "$RUNNER_TEMP/bench"
65+
66+
- name: Render
67+
run: |
68+
uv run python -m benchmarks.report results.json --out summary.md
69+
cat summary.md >> "$GITHUB_STEP_SUMMARY"
70+
71+
- uses: actions/upload-artifact@v4
72+
if: always()
73+
with:
74+
name: benchmark-${{ github.ref_name }}
75+
path: |
76+
results.json
77+
summary.md
78+
retention-days: 90
79+
80+
# docs/ is the durable home: artifacts expire, and one absolute number
81+
# with nothing to compare it against says very little. The page is
82+
# already served by Pages from docs/, so this needs no extra machinery.
83+
- name: Publish to docs
84+
if: github.ref_type == 'tag' || inputs.publish
85+
run: |
86+
git config user.name "github-actions[bot]"
87+
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
88+
# A tag build is on a detached HEAD; the page belongs on main.
89+
git fetch origin main
90+
git checkout -B main origin/main
91+
uv run python -m benchmarks.report results.json --docs docs --publish
92+
git add docs/BENCHMARKS.md docs/benchmarks
93+
if git diff --cached --quiet; then
94+
echo "nothing to publish"; exit 0
95+
fi
96+
git commit -m "Benchmark results for ${{ github.ref_name }} [skip ci]"
97+
# Another job may have landed on main in the meantime.
98+
for attempt in 1 2 3; do
99+
if git push origin main; then exit 0; fi
100+
git pull --rebase origin main
101+
done
102+
echo "could not push benchmark results"; exit 1
103+
104+
# Best effort, and last: releases here are cut by hand, so this job may
105+
# well run before one exists. The docs page is authoritative either way.
106+
- name: Append to the release notes
107+
if: github.ref_type == 'tag'
108+
continue-on-error: true
109+
env:
110+
GH_TOKEN: ${{ github.token }}
111+
run: |
112+
tag="${{ github.ref_name }}"
113+
gh release view "$tag" >/dev/null 2>&1 || {
114+
echo "no release for $tag yet; skipping"; exit 0
115+
}
116+
gh release view "$tag" --json body -q .body > body.md
117+
# Idempotent: replace any block this job wrote before.
118+
python - <<'PY'
119+
import pathlib, re
120+
body = pathlib.Path("body.md").read_text()
121+
summary = pathlib.Path("summary.md").read_text()
122+
block = (
123+
"<!-- benchmark:start -->\n\n"
124+
"<details><summary>Benchmark vs anndata/scanpy</summary>\n\n"
125+
f"{summary}\n</details>\n\n<!-- benchmark:end -->"
126+
)
127+
pattern = re.compile(
128+
r"<!-- benchmark:start -->.*?<!-- benchmark:end -->", re.S
129+
)
130+
body = (
131+
pattern.sub(block, body)
132+
if pattern.search(body)
133+
else body.rstrip() + "\n\n" + block + "\n"
134+
)
135+
pathlib.Path("body.md").write_text(body)
136+
PY
137+
gh release edit "$tag" --notes-file body.md

‎.gitignore‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,9 @@ htmlcov/
1010
.pytest_cache/
1111
pytest-results*.xml
1212
compat-results.xml
13+
14+
# Benchmark working directory and results -- the defaults of
15+
# `python -m benchmarks.run`, both written into the repo root.
16+
.bench/
17+
results.json
18+
summary.md

‎CHANGELOG.md‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,117 @@
33
Notable changes to `adata-cli`. Versions are `MAJOR.MINOR.PATCH`; tags carry no
44
`v` prefix.
55

6+
## Unreleased
7+
8+
### Fixed
9+
10+
- **`concat --merge` never finished on a real store.** Aligning a var column
11+
onto the target index re-read the whole column from disk once per target
12+
variable, so the cost was quadratic: at 36,601 variables a merge that should
13+
take a fraction of a second ran for hours at 100% CPU with the output file
14+
never growing past its header. Reported against 0.5.1 (REQ-71798), where 12
15+
of 13 pipeline tasks had to be killed after 98 minutes. The column is now
16+
read once per input, and `--merge first` / `--merge only`, which decide on
17+
presence alone, read no column values at all.
18+
- **`concat` was quadratic in the number of categories** in an obs column.
19+
Category merging probed a list rather than a dict: 2,096,128 string
20+
comparisons to union 1,024 categories, and around 5e9 for a 100k-category
21+
column. Found by the new guards.
22+
- **`split --by` was quadratic**, O(n_rows x n_groups). `group_indices` grouped
23+
rows with `np.nonzero(values == label)` inside a loop over distinct labels,
24+
rescanning each chunk once per label: at 4,096 rows, 16,384 elements scanned
25+
for 4 groups and 1,048,576 for 256. A million cells split by a thousand
26+
samples is ~10^9 comparisons. One `np.unique` pass per chunk makes it flat
27+
in the group count. Found by the new guards; order of first appearance,
28+
which names the output files, is unchanged.
29+
- **`concat` built a Python object per row** for nullable and string obs
30+
columns, then walked the list twice more. Filling a typed buffer by slice
31+
removes three full passes over every such column.
32+
33+
### Added
34+
35+
- **Complexity guards in the test suite** (`tests/test_performance.py`).
36+
Cost regressions now fail at merge time. They count operations rather than
37+
seconds -- h5py and zarr reads, Zarr store traffic, Python allocation and
38+
executed lines -- and assert that successive increments grow no faster than
39+
linearly, so nothing here can fail because a CI runner was busy. See
40+
[docs/TESTING.md](docs/TESTING.md#performance). Every subcommand is covered:
41+
`ls`, `view`, `create`, all five `export` and all five `import` variants,
42+
`split` on both axes, and the `concat` options nothing else reached.
43+
Two claims are now enforced rather than described -- `view` and `ls` read
44+
**zero** data elements at any store size, and streaming stays far below the
45+
input curve at a fixed `--chunk`.
46+
- **Copying variable-length strings ignored its own read budget.** The width
47+
of a vlen element was assumed to be 64 bytes, because h5py reports the
48+
itemsize of a pointer, so the step was the same 524,288 elements whatever
49+
the data held: 2 GiB per read at 4 KiB elements against a stated 32 MiB
50+
budget, and for any array shorter than that step, the whole array in one
51+
go. Copying 200,000 strings of 4 KiB peaked at 827 MB. The width is now
52+
sampled from the first 256 elements. Reported by an automated review on
53+
PR #14 and confirmed by measurement; `uns` can hold arbitrary text, so this
54+
was not a width the layer could assume.
55+
- **Peak RSS in the benchmark was floored by the runner's own memory on Linux.**
56+
A forked child inherits its parent's resident pages and `execve` folds that
57+
into the `maxrss` the kernel reports, so every contender would have measured
58+
at least what `benchmarks/run.py` used to build the fixtures — around
59+
200 MB — and the tables would have read "everything costs about the same".
60+
Commands are now forked from a small shim: with a 330 MB parent, a no-op
61+
child goes from 326 MB to 8 MB. Caught by `test_benchmark_harness.py`, which
62+
exists for exactly this. The published figures were measured on macOS, which
63+
resets the high-water mark at exec, and are unchanged.
64+
- **A comparative benchmark** (`benchmarks/`), run on every tag against
65+
anndata and against scanpy where scanpy has a real equivalent. Reports peak
66+
RSS, wall time and output size; publishes to
67+
[docs/BENCHMARKS.md](docs/BENCHMARKS.md) and the release notes. Report-only
68+
-- it never fails a build. Fifteen cases, covering every command with a real
69+
baseline, including `h5ls -r` for `ls` and the rows where adata-cli is the
70+
slower of the two.
71+
- **`--merge drop` and `--uns-merge drop` are accepted.** `drop` was already
72+
the documented default behaviour but was rejected as a value, so a config
73+
could not state it explicitly.
74+
75+
## 0.5.1
76+
77+
Makes the container image usable from Nextflow, and stops `copy_dataset`
78+
reading one row at a time from row-chunked stores.
79+
80+
### Fixed
81+
82+
- **Copying a row-chunked store was dominated by read latency.** The read step
83+
was the source's chunk height verbatim, so a store chunked `(1, n_cols)` was
84+
copied one row per read. On a network filesystem (Lustre, NFS) each read is a
85+
round-trip, so a million-row copy spent nearly all of its time waiting. Reads
86+
are now sized to a 32 MiB budget, rounded down to a whole number of source
87+
chunks. A `(1_000_000, 30_000)` float32 store chunked `(1, 30_000)` goes from
88+
1 row per read to 279.
89+
- Read sizing no longer trusts `itemsize` for variable-length strings. h5py
90+
reports 8 there because the value is a pointer, which overestimated the row
91+
count by an order of magnitude and broke the memory bound.
92+
93+
### Container
94+
95+
- **The image could not be used from a Nextflow process.** Nextflow requires
96+
`/bin/bash` to be the container entrypoint, so `ENTRYPOINT ["adata"]` made
97+
every Docker- and Podman-backed task fail with `No such command
98+
'/bin/bash'`. Apptainer was unaffected, as `singularity exec` ignores the
99+
entrypoint.
100+
- **Task metrics were silently lost.** `procps` is absent from the base image,
101+
so Nextflow could not run `ps` to collect them. The required tool set
102+
(`bash`, `ps`, `awk`, `date`, `grep`, `sed`, `tail`, `tee`) is now installed
103+
and asserted at build time.
104+
- `PYTHONNOUSERSITE` is set, so a bind-mounted `$HOME` under Apptainer can no
105+
longer shadow the image's virtualenv with the user's `~/.local` packages.
106+
- `XDG_CACHE_HOME` points at `/tmp`, so the image tolerates being run under an
107+
arbitrary UID with no writable `$HOME`.
108+
- Added a `.dockerignore`. Local builds were copying the host's `.venv`,
109+
`.git` and `.pytest_cache` into the image.
110+
111+
### Changed
112+
113+
- **The image no longer sets an entrypoint, so the command must be named
114+
explicitly:** `docker run IMAGE adata view file.h5ad`, where `docker run
115+
IMAGE view file.h5ad` previously worked.
116+
6117
## 0.5.0
7118

8119
Renamed from `h5ad` to `adata-cli`, restored compatibility with current

‎Dockerfile‎

Lines changed: 57 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,33 @@
11
# Base image: Python 3.12 + uv preinstalled (Debian slim)
22
FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim
33

4-
ENV UV_NO_DEV=1
4+
# PYTHONNOUSERSITE: Apptainer bind-mounts the host $HOME by default, so a user's
5+
# ~/.local/lib/python3.12/site-packages would otherwise shadow this venv.
6+
# XDG_CACHE_HOME: Nextflow is commonly configured with `-u $(id -u):$(id -g)`,
7+
# which leaves the container with no writable $HOME.
8+
# UV_COMPILE_BYTECODE: bake .pyc at build time, so nothing writes to a
9+
# read-only rootfs on first import.
10+
ENV UV_NO_DEV=1 \
11+
UV_COMPILE_BYTECODE=1 \
12+
PYTHONNOUSERSITE=1 \
13+
PYTHONDONTWRITEBYTECODE=1 \
14+
PYTHONUNBUFFERED=1 \
15+
XDG_CACHE_HOME=/tmp/.cache
516

6-
WORKDIR /cli
7-
8-
# Copy the project files (from the GitHub Actions checkout context)
9-
COPY . .
10-
11-
# --locked asserts that uv.lock is in sync with pyproject.toml, so an image
12-
# can never be built from a lockfile that drifted.
13-
RUN uv sync --locked
17+
# procps supplies `ps`, which Nextflow needs to collect per-task metrics.
18+
# curl, unzip and ca-certificates fetch duckdb below, and are left in place
19+
# rather than purged: pipeline scripts routinely reach for curl, and TLS
20+
# roots are worth having in any container that may touch the network.
21+
RUN apt-get update \
22+
&& apt-get install -y --no-install-recommends \
23+
ca-certificates curl procps mawk unzip \
24+
&& rm -rf /var/lib/apt/lists/*
1425

1526
# duckdb, for the filtering workflows in the docs: export obs to CSV, query it,
1627
# feed the names back to `adata subset --obs`. A single static binary, so it
1728
# needs no venv and cannot conflict with the project's dependencies.
1829
ARG DUCKDB_VERSION=v1.1.3
19-
RUN apt-get update \
20-
&& apt-get install -y --no-install-recommends ca-certificates curl unzip \
21-
&& ARCH="$(dpkg --print-architecture)" \
30+
RUN ARCH="$(dpkg --print-architecture)" \
2231
&& case "$ARCH" in \
2332
amd64) DUCKDB_ARCH=amd64 ;; \
2433
arm64) DUCKDB_ARCH=aarch64 ;; \
@@ -28,13 +37,43 @@ RUN apt-get update \
2837
"https://github.com/duckdb/duckdb/releases/download/${DUCKDB_VERSION}/duckdb_cli-linux-${DUCKDB_ARCH}.zip" \
2938
&& unzip -q /tmp/duckdb.zip -d /usr/local/bin \
3039
&& chmod +x /usr/local/bin/duckdb \
31-
&& rm /tmp/duckdb.zip \
32-
&& apt-get purge -y curl unzip \
33-
&& apt-get autoremove -y \
34-
&& rm -rf /var/lib/apt/lists/*
40+
&& rm /tmp/duckdb.zip
41+
42+
# Fail the build, rather than every Nextflow task, if the base image ever drops
43+
# one of the tools Nextflow requires in a task container.
44+
RUN set -eu; for t in bash ps awk date grep sed tail tee; do \
45+
command -v "$t" >/dev/null || { echo "missing required tool: $t" >&2; exit 1; }; \
46+
done
47+
48+
WORKDIR /cli
49+
50+
# Copy the project files (from the GitHub Actions checkout context)
51+
COPY . .
52+
53+
# --locked asserts that uv.lock is in sync with pyproject.toml, so an image
54+
# can never be built from a lockfile that drifted.
55+
#
56+
# uv honours XDG_CACHE_HOME, so the sync leaves a root-owned package cache at
57+
# /tmp/.cache -- which made the variable self-defeating, as a task running
58+
# under an arbitrary UID then could not write to the very path it advertises.
59+
# Clear it and leave an empty world-writable directory behind. Done in this
60+
# same layer because a later `rm` would mask the files without reclaiming
61+
# them; that reclaims about 9 MB, the cache being mostly hardlinks into the
62+
# venv rather than separate copies.
63+
RUN uv sync --locked \
64+
&& rm -rf /tmp/.cache /tmp/uv-*.lock \
65+
&& mkdir -p /tmp/.cache \
66+
&& chmod 1777 /tmp/.cache
3567

3668
# Put the project venv on PATH so `adata` is directly runnable
3769
ENV PATH="/cli/.venv/bin:${PATH}"
3870

39-
ENTRYPOINT ["adata"]
40-
CMD ["--help"]
71+
# No ENTRYPOINT on purpose: Nextflow requires /bin/bash to be the container
72+
# entrypoint, so the image must not set one of its own. This is why invocations
73+
# spell out the command: `docker run IMAGE adata view file.h5ad`.
74+
#
75+
# Deliberately NOT set here: OMP_NUM_THREADS / OPENBLAS_NUM_THREADS. NumPy's
76+
# BLAS sizes its thread pool to the whole host, which oversubscribes a shared
77+
# LSF node. This workload is streaming I/O, so capping it would cost nothing --
78+
# but it belongs in the pipeline's `env` scope, not baked into the image.
79+
CMD ["adata", "--help"]

0 commit comments

Comments
 (0)