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>
_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>
Nextflow-compatible image, faster streaming reads, release 0.5.1
…ariable `_write_var` aligned each candidate var column onto the target index with `tuple(read_str_all(group[name])[i] for i in where)`. Only the outermost iterable of a generator expression is evaluated eagerly, so `read_str_all` ran once per target variable -- a full read of the column from disk, per element of the same column. The cost is quadratic in the number of variables. Measured here on two synthetic inputs with two var columns: 0.48 s at 500 vars, 1.40 s at 1,000, 4.55 s at 2,000, 16.57 s at 4,000. Extrapolated to the 36,601 vars of REQ-71798 that is tens of minutes to hours of pure CPU with the output file never growing past the header it wrote first, which is exactly what was reported against 0.5.1: 12 of 13 pipeline tasks killed after 98 minutes, and byte counts identical between `--merge same` and `--merge first`. Reading the column once makes the same case 0.02 s, and 0.25 s at the full 36,601 x 8,766 of the ticket. `first` and `only` decide on presence alone and now read no column values at all, which is why their cost matched `same` before. Also accept `--merge drop` / `--uns-merge drop`. `drop` is the documented default behaviour but was rejected as a value, so a config could not state it. The three new tests count full-column reads rather than timing the merge: the defect is a complexity bug, invisible to every existing concat test because they all use two or three variables, and a wall-clock assertion would be flaky on shared CI. `test_concat_merge_same_reads_each_var_column_once_per_input` is parametrised over 4 and 64 variables so that a cost which grows with the var count fails the second case; against the old code it reports 24 and 384 reads where 6 are expected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The --merge hang was not a one-off bug but a defect class the suite could not
see: output correct, cost wrong. Every fixture here is a few hundred elements
and nothing measured cost, so no test could have failed.
tests/perf_counters.py instruments four seams, all verified against the pinned
h5py 3.15.1 and zarr 3.1.5: h5py.Dataset.__getitem__, zarr.Array.__getitem__,
and LocalStore.get/set/delete (coroutines, wrapped as such). Patching the
libraries rather than adding a seam inside src/adata is deliberate -- an
in-repo helper would only see the call sites that remembered to use it, and
the matrix paths in subset.py and concat.py slice the backend objects
directly. Elements are counted, not just calls, so a vectorised-but-quadratic
read is caught too. The known bypasses (read_direct, np.asarray(dataset),
asstr) are documented, and two canary tests fail loudly if a hook stops
firing, since otherwise every ratio below would pass on zeros.
The invariant compares successive increments rather than raw counts:
d1 = c(4n) - c(n); d2 = c(16n) - c(4n); assert d2 <= 6 * d1
The increment form cancels any fixed setup cost exactly, so there is no slack
constant to tune and no floor for a small-coefficient quadratic to hide under.
At 4x spacing the ratio is 4.0 for linear work, 4.4 for n log n, 8 for n**1.5
and 16 for quadratic, so 6 sits in the gap with room either side; a test
asserts that calibration rather than leaving it as a comment. Each guard
scales exactly one axis -- n_var, n_obs, n_inputs, n_columns, n_categories --
because scaling two at once makes legitimate work look quadratic.
Reintroducing the REQ-71798 line makes the n_var guard fail at ratio 15.9 on
all four merge strategies.
Writing the guards turned up two more defects of the same class, both fixed
here:
_concat_categorical unioned categories with `if category not in categories`
on a list -- O(k^2). Neither instrument above sees it: the category lists are
read once either way, and `x not in lst` is a single bytecode, so the
quadratic lives inside C-level list membership. Counting string comparisons
via a str subclass is what makes it visible: 2,096,128 comparisons at k=1024,
and around 5e9 for a 100k-category obs column. A dict takes it to 0.
_concat_masked filled a Python list of length n_obs one element at a time and
then walked it twice more. A typed numpy buffer filled by slice takes it from
1.21x the executed Python lines of the numeric path to 1.00x. The string path
is 1.35x and stays there -- read_str_all materialises Python str objects,
which is inherent to reading strings rather than a per-row loop -- so its
guard is set at that measured level with the reason recorded.
Also registers the perf marker (--strict-markers is on) and notes what these
tests deliberately do not claim: obs columns are read whole, so peak
allocation is O(n_obs) and not O(chunk). The streaming guarantee holds for X,
not for obs annotation. benchmarks/ will report that curve.
853 tests pass, coverage 92.6%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complements the complexity guards rather than duplicating them. The guards count operations, are deterministic, and gate merges; this measures wall time and peak RSS at realistic size, runs on tags, and never fails a build -- timing on a shared runner is too noisy to gate a release on, and a benchmark that can block a publish stops being run. Peak RSS is the headline. adata-cli exists so that memory is set by --chunk rather than input size, and for data that fits in RAM loading the whole thing is frequently faster; a table reporting only wall time would misrepresent the tool in the direction of flattery and then in the direction of failure. Measurement (benchmarks/_measure.py): os.wait4, not resource.getrusage. RUSAGE_CHILDREN is a running maximum over every child a process has reaped, so a 400 MB case followed by a 1 kB one reports 400 MB twice and every later row inherits the largest earlier peak. Output goes to temporary files rather than pipes, because communicate() reaps the child and there is then nothing for wait4 to report. ru_maxrss is normalised -- KiB on Linux, bytes on macOS. RLIMIT_AS at 12 GiB on every child. Cases the in-memory baseline cannot survive are the point of the comparison, but an uncontained OOM kills the runner agent and the job ends with no report at all; with a ceiling it is a row that says "out of memory" at a limit we can state. A timeout records the output size at the kill, which is how the 0.5.1 hang actually presented -- 1,489,960 bytes, never growing -- and distinguishes it from slow progress. tests/test_benchmark_harness.py covers exactly this and nothing else. If peak RSS were attributed to the wrong process, every published table would be wrong and would still look plausible. Fairness rules, written into cases.py and docs/TESTING.md because this is what decays first: use the best idiom the baseline has (read_elem, backed mode) and never a strawman full load as the primary row; pin compression on both sides, since adata-cli forwards the source's settings while write_h5ad defaults to none; print n/a with a reason where scanpy or concat_on_disk has no equivalent, because an omitted row reads as an oversight; include a startup floor, as the CLI costs 0.3-1 s to import and scanpy 3-8 s; say whether the page cache was dropped; and leave the rows where anndata wins exactly as measured. _concat_csr loops per row in Python and scipy's C vstack will often beat it on time at several times the memory -- that trade is the argument for this tool, and hiding it would make the table worthless. Baselines run from venvs built up front rather than `uv run --with`. The latter is right for reference_stores.py, where fixture cost is irrelevant, and wrong here: the first invocation would put hundreds of megabytes of wheel downloads into the measured wall time and uv's own memory into the measured peak. scanpy stays out of uv.lock and out of the image either way. Publishing goes to docs, not an orphan branch: docs/benchmarks/<tag>.json for the series and docs/BENCHMARKS.md for the rendered page, both already served by Pages from docs/, plus the step summary and a best-effort idempotent append to the release notes. Artifacts expire in 90 days and one absolute number with nothing to compare against says very little, so the durable series is the part that matters. Note for review: the docs commit is the one outward-facing write here -- a tag build is on a detached HEAD, so the job commits to main as the bot with [skip ci] and rebase-retries on a race. Its own workflow file rather than a job in publish.yml: at the ci tier this takes the better part of an hour, and hanging that off the release graph would either delay the PyPI publish or paint the release run red for a report. It benchmarks the checked-out source, not the published wheel, which would mean waiting on publish-pypi and then on index propagation for a measurement that comes out the same. 861 tests pass, coverage 92.6%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ci tier is much cheaper than the plan assumed: 580 MB of fixtures and three cases ran end to end in 43 seconds, so the full set is minutes rather than the hour the workflow allows. The 90-minute timeout stays as headroom for the large tier; calling it an estimate would have been wrong. Records the numbers that run produced, including that adata-cli's 202 MB peak is not flat in input size -- obs columns are read whole and a dense block is --chunk x n_var. Better to state that next to the figures than to let the page imply a guarantee the code does not yet meet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g it The previous round pointed both mechanisms almost entirely at concat. ls, view, create, the five exports and the five imports had no cost coverage at all. Extending to them needed three additions to the instruments, and turned up a third defect of the same class as the first two. split --by was O(n_rows x n_groups). core/select.py group_indices grouped rows with `np.nonzero(values == label)` inside a loop over distinct labels, so each chunk was rescanned once per label: measured at 4,096 rows, 16,384 elements scanned for 4 groups and 1,048,576 for 256 -- exactly n_rows per group. A million cells split by a thousand samples is 10^9 comparisons, and split would have looked like a hang for the same reason concat --merge did. np.unique with return_index and return_inverse does it in one pass per chunk: 16,388 elements at 4 groups and 16,640 at 256, flat to within 1.5%. Order of first appearance is preserved through argsort on the first-occurrence indices, because it names the output files. The existing split guard passed throughout. The chunk is already in memory, so no read counter moves -- the same blind spot as _concat_categorical, and the same lesson: one instrument is never enough. Three additions to tests/perf_counters.py: count_scanned_elements counts what is handed to numpy's scanning primitives. It is a floor, not a measurement -- an operator like `values == label` dispatches to the ufunc in C and never passes the patched np.equal -- so the guard using it asserts a lower bound, and the docstring says exactly what is and is not visible. HDF5 writes were not counted at all, which made every import and create guard silently vacuous at zero. Both Dataset.__setitem__ and Group.create_dataset are now hooked; the latter matters because create_dataset(name, data=...) writes its payload at creation and never touches __setitem__. assert_independent_of asserts cost does not grow at all, and assert_grows_slower_than_input asserts it grows by at least some factor less than the input. assert_grows_linearly could only catch super-linear growth, and would have accepted a 64x increase in a command that is supposed to read nothing. The vacuity floor in assert_grows_linearly moved from `d1 >= mid` to half the increment: work that is exactly one operation per element -- export dict reads each key once -- gives 0.75 * mid and was being rejected as unmeasurable. Two claims are now enforced rather than asserted in prose: view and ls read zero data elements, at 64 rows and at 4,096. Not "grows slowly" -- zero, an exact count needing no tolerance. A control test exports the same fixture to prove there was data there to read, so the zero cannot pass by accident, and injecting a single column read into show_info fails four of the ten cases with the offending dataset named. Streaming is bounded well below the input, which is weaker than the README implies and is what the measurements support. Over a 256x span at fixed chunk: export array 1.6x, export dataframe 2.2x, export sparse 6.5x, subset 46x. Only export array is close to flat, so only it is asserted as such, and subset's guard is deliberately the loosest -- obs columns are materialised per column, a known gap that benchmarks/ reports rather than this hiding. Streamed export sparse is also asserted to stay under a quarter of what --in-memory costs, measured in the same run so the factor holds anywhere. Coverage added for view, view --types, ls, ls --long, ls --plain, create (generated names and name file), export dataframe by rows and by columns, export array, export sparse both paths, export dict, export image, import dataframe/array/sparse/dict/image, concat --label, concat --index-unique and split --axis var. 895 tests pass, coverage 92.71%. The perf file is ~55 s, most of it building 65,536-row fixtures, so those three carry the previously unused slow marker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six cases added, chosen by whether a real baseline exists: ls, create, export-array, export-sparse, import-dataframe and concat-outer. export image, export dict, import image and import dict are deliberately left out -- no library offers them, so the rows would only ever read n/a while adding runtime to every tag. The complexity guards cover them instead. Two of the new rows are the reason the report is framed as "peak RSS against wall time" rather than as a leaderboard. At the ci tier, export sparse streams a 50,000 x 20,000 matrix in 68 MB and takes 10.8 s where loading it whole takes 759 MB and 1.9 s; and h5ls -r lists the file in 0.03 s and 7 MB against our 0.36 s and 63 MB, being C rather than a Python process that has to import typer, rich, h5py and zarr first. Both are published. A benchmark that showed only the rows we win would not be worth the runtime. The rest of the ci run: create 78 MB / 0.28 s against 2,225 MB / 2.85 s, import-dataframe 107 MB / 0.41 s against 571 MB / 1.84 s, concat-outer 202 MB / 2.60 s against 1,362 MB / 4.02 s in memory and 436 MB / 2.24 s for concat_on_disk, which is slightly the faster of the two. Harness changes the new cases needed: Baseline environments now install dask. concat_on_disk imports it to concatenate a dense element and raises ModuleNotFoundError without it, which surfaced as soon as the fixtures gained an obsm. Giving the baseline its best idiom is the standing rule; measuring a library crippled by a missing optional dependency would be measuring our own setup. A case can declare a sidecar input, built once from the real store, so import-dataframe reads a CSV the file could plausibly have held rather than an invented one. Contenders whose binary is absent -- h5ls is often not installed -- are recorded as n/a with the reason rather than crashing the run. create takes its shape from the tier. Hardcoding 50,000 x 20,000 made the smoke tier allocate a 4 GB dense array, so "smoke" was not smoke: its peak went from 2,324 MB to 64 MB once the shape followed the tier. Fixtures gained a 50-column obsm, without which export array and import array had nothing of realistic width to move. docs/TESTING.md gets the three invariants and when each applies, the two write seams and why both are needed, count_scanned_elements and the precise statement of what it cannot see, and a per-command coverage table so the next command's author knows what is expected. The measured streaming growth figures are recorded there too -- 1.6x, 2.2x, 6.5x, 46x over a 256x span -- because the guards are set from them. 895 tests pass, coverage 92.71%. All 15 benchmark cases run clean at smoke. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three problems with docs/BENCHMARKS.md, all of which would have shown up first on the next tag. Nothing linked to it. Neither README.md nor docs/index.md mentioned the page at all, so the one document that says what the streaming claim actually costs was reachable only by guessing the URL. Both now link it, and the README's "streaming access to very large stores" bullet points straight at it -- including at the rows where loading the file outright is faster. `publish()` overwrote the whole page with bare tables. Every word explaining what the numbers mean would have been deleted the first time the workflow ran. The prose now lives in benchmarks/page_template.md with a `<!-- results -->` marker, `build_page` fills it, and docs/BENCHMARKS.md is generated from that same template so the words exist once. A test asserts the marker is still there and that the framing and the trailing sections survive a republish. The rendered results opened with their own H1 and repeated the "peak RSS is the headline" paragraph the template already carries. Results are now an H2 under the page's own title, per-case tables are H3, and the duplicated framing is gone -- the published page has exactly one H1, which the test checks. The page itself now says what is measured, which four commands are deliberately absent and why, and carries the caveat that peak memory is not flat in input size: 1.6x to 46x over a 256x span depending on the command. Better for that to be on the page than discovered by a user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rge-hang # Conflicts: # CHANGELOG.md
Fallout from resolving the CHANGELOG merge by dropping conflict markers. Co-Authored-By: Claude Opus 5 <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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c38c41187
ℹ️ 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".
Comment on lines
+469
to
+470
| if itemsize <= 0 or getattr(dtype, "kind", None) in ("O", "T"): | ||
| itemsize = VLEN_ELEMENT_BYTES |
There was a problem hiding this comment.
When copy_dataset encounters a variable-length string array, this fixed 64-byte estimate makes a one-dimensional dataset use chunks of up to 524,288 elements, regardless of their actual lengths. Because copy_tree also copies arbitrary arrays under locations such as uns, entries can legitimately be much larger than cell or gene names; for example, 1 KiB strings make one read allocate roughly 512 MiB, while larger text can exhaust memory entirely. This regresses the previous source-chunk-sized streaming behavior and defeats the stated memory bound, so variable-length arrays need a conservative row cap or sizing based on stored payload size.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Confirmed, and worse than described — fixed in e8c24fb.
Measured on 200,000 vlen strings, varying only the element width:
| element width | peak allocation |
|---|---|
| 16 B | 11.5 MB |
| 256 B | 59.4 MB |
| 1 KiB | 213.0 MB |
| 4 KiB | 827.4 MB |
against a stated 32 MiB budget. The extra detail is that the computed step (524,288 elements) exceeded the row count in all four cases, so the whole array was read in one go — the opposite of what the budget exists for.
The width is now sampled from the first 256 elements rather than assumed; VLEN_ELEMENT_BYTES remains the fallback and a floor. Two guards added in tests/test_performance.py: one checking the arithmetic directly (step × width within budget at 16/256/4096 B), one checking it in practice on a 164 MB array — the latter fails against the previous code at 165 MB where the bound is 100 MB.
You were right that uns is the problem case: cell and gene names really are under 64 bytes, but copy_tree carries arbitrary text, so it was never a width this layer could assume.
…nner's
CI failed one test on both interpreters, and it was not a flaky threshold.
test_peak_rss_is_attributed_to_the_right_child reported 146 MB for a child
that allocated 1 KB, after a 410 MB child. Its docstring says why it exists:
if peak RSS were attributed to the wrong process, every table this project
publishes would be wrong and the numbers would still look plausible.
A uniform interpreter floor was ruled out by the data -- at 140 MB the big
child would have measured 540 MB, not 410. Reproduced under python:3.12-slim:
parent 14.7 MB -> no-op child 11.8 MB
parent 329.6 MB -> no-op child 326.4 MB
parent 329.6 MB -> via shim 8.1 MB
On Linux a forked child inherits its parent's resident pages, and execve
folds that pre-exec high-water mark into the accumulated maxrss that wait4
reports. A child of a fat parent cannot appear small. macOS resets it at
exec, which is why this passed locally and failed on CI -- the one platform
the benchmark actually runs on.
This was never only a test problem. run.py imports anndata, pandas and numpy
to build fixtures in the same process that calls measure(), so on the runner
every contender would have been floored at roughly 200 MB. The headline
result -- 202 MB against 1,847 MB for ad.concat -- would have collapsed to
"everything costs about the same", which is precisely the claim the benchmark
exists to test, failing silently in the flattering direction.
measure() now re-invokes benchmarks/_measure.py as a subprocess and that
freshly-exec'd interpreter, about 8 MB, forks the command being measured. The
in-process logic is unchanged, renamed _measure_here; main() grew the
--timeout, --memory-limit, --cwd and --env options it needs to carry the
call. RLIMIT_AS still applies via the shim's preexec_fn, so OOM containment
for the large tier is intact.
Dropping preexec_fn to get posix_spawn was the obvious alternative and does
not work: the middle row above measures 329.5 MB that way too. It would also
have given up the address-space ceiling.
The test now asserts the property rather than a ratio. It holds 300 MB of
ballast for its duration, measures a no-op baseline child, and requires both
that the baseline is small in absolute terms -- the absence of an inherited
floor -- and that the small child resembles the baseline rather than the big
one before it. Against the old code on Linux it fails at 342 MB for a no-op
child; the previous form only failed when the floor happened to exceed a
quarter of the largest child.
Two things found on the way:
build_environments only checked that the interpreter existed, so a reused
--work directory kept whatever was installed first. Adding dask to
ENVIRONMENTS had no effect on an existing tree and concat_on_disk went on
raising ModuleNotFoundError as though that were a finding about anndata. The
package list is now recorded beside the venv and triggers a rebuild when it
changes, with --clear so the rebuild does not die on the existing tree.
A command that does not exist is classified n/a by the shim with the reason,
rather than surfacing as a traceback; the duplicate check in run.py is gone.
The refuse-to-overwrite guard stays in the parent so it still raises.
Re-measured at the ci tier afterwards: peak RSS is unchanged to within a
megabyte across every documented case, as expected since those figures came
from macOS. They stand.
905 tests pass, coverage 92.78%. All 15 benchmark cases clean at smoke; the
harness tests pass under python:3.12-slim, where they now also cover the
RLIMIT_AS path that macOS skips.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Flagged by an automated review on PR #14, on code that came in with the main merge -- 4859445, released in 0.5.1. Confirmed by measurement, and worse than the review described. _row_bytes assumed 64 bytes for a variable-length element, because h5py reports the itemsize of a pointer. An assumption is not a bound. The step was therefore the same 524,288 elements whatever the data actually held: 16 B strings, 200k rows -> 11.5 MB peak 256 B -> 59.4 MB 1 KiB -> 213.0 MB 4 KiB -> 827.4 MB against a stated 32 MiB budget and because the computed step exceeded the row count in every one of those cases, the whole array was read in a single go -- the opposite of what the budget is for. Real cell and gene names do sit under 64 bytes, but copy_tree carries arbitrary `uns` content, so this is not a width the storage layer can assume. The width is now sampled from the first 256 elements, one small read against a copy about to stream the whole array. VLEN_ELEMENT_BYTES stays as the fallback when sampling is not possible, and as a floor so a column of empty strings cannot produce an unbounded step. _row_bytes keeps its old two- argument form via a default, so the existing 0.5.1 tests still describe it. Two guards, because they catch different things. The parametrised one checks the arithmetic directly -- step x width must stay inside the budget at 16, 256 and 4096 bytes -- which is exact and costs nothing. The slow one checks it in practice on a 164 MB array, and is the one that fails against the old code, at 165 MB where the bound is 100 MB. This is the path the rest of test_performance.py did not reach, and every `copy:` task in subset and every uns entry goes through it. The coverage claimed in the previous commit was not as complete as it read. 909 tests pass, coverage 92.79%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
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.
Why
adata concat --mergehung for hours on a real store (REQ-71798, 0.5.1): 12 of 13 pipeline tasks killed after 98 minutes, output frozen at 1,489,960 bytes. One line re-read a whole var column from disk once per target variable.1,029 tests missed it, and were never going to. Every fixture in the suite is at most a few hundred elements and nothing measured cost — so an entire defect class, output correct and cost wrong, had no test that could fail. For a tool whose selling point is that memory is set by chunk size rather than input size, that is the worst possible blind spot.
This PR fixes the reported bug, closes the gap that hid it, and finds two more defects of the same class on the way.
Three quadratic defects, all now guarded
_write_var_concat_categoricalnot inon a listgroup_indices(split --by)Each needed a different instrument. That is the main lesson here: a read counter sees the first and neither of the others.
_concat_maskedalso built a Python object per obs row and walked it twice more; a typed buffer filled by slice takes it from 1.21× the numeric path's executed lines to 1.00×.Two mechanisms, deliberately separate
tests/test_performance.pybenchmarks/workflow_dispatchA test that can fail because a runner was busy does not belong in a merge gate, so the guards never touch the clock. This extends the house style already in
test_commands_phase2.pyrather than inventing a second one.The guards
Counters hook h5py and zarr themselves — reads, writes, and Zarr store traffic — rather than adding a seam inside
src/adata, which would only see the call sites that remembered to use it. Three invariants:d2 <= 6·d1on increments across n/4n/16n. The increment form cancels fixed setup cost exactly, so there is no slack constant to tune. A test asserts that calibration rather than leaving it as a comment.viewandlsread zero data elements at any store size. Not a ratio; an exact number. A control test proves the fixture had data to read, and injecting one column read intoshow_infofails four cases and names the culprit dataset.--chunk, held to a stated factor better than the input.Every subcommand is covered:
ls,view,create, all fiveexportand all fiveimportvariants,spliton both axes,concatincluding--labeland--index-unique.Reintroducing the REQ-71798 line fails the n_var guard at ratio 15.9 on all four merge strategies.
The benchmark
Fifteen cases against anndata, and scanpy where it has a real equivalent. Report-only, published to
docs/BENCHMARKS.mdand the release notes on every tag. Measured at 50,000 × 20,000:concat-innercreateexport-sparselsh5ls -r)The last two rows are published deliberately.
export sparsestreams in a tenth of the memory and takes five times as long;h5lsbeats us on both axes because it is C and does not start a Python interpreter. A benchmark showing only the rows we win would not be worth the runtime.Peak RSS is measured with
os.wait4, notgetrusage—RUSAGE_CHILDRENis a running maximum and would attribute one case's peak to every later one. Children run under a 12 GiBRLIMIT_ASso a baseline that cannot cope is a reported row rather than a dead runner.Honest caveat
Peak memory is not flat in input size. Over a 256× span at fixed chunk it grows 1.6× for
export array, 2.2× forexport dataframe, 6.5× forexport sparseand 46× forsubset— far below the input curve, but not constant, because obs columns are read whole. So the guards assert what is true rather than what the README implies, anddocs/BENCHMARKS.mdstates it plainly. Worth knowing from our own measurement rather than from a user.Also in here
--merge drop/--uns-merge dropare accepted.dropwas the documented default but was rejected as a value, so a config could not state it.mainmerged in.devwas 5 commits behind and missing the whole 0.5.1 release, so this branch's CHANGELOG sat on top of 0.5.0 andpyproject.tomlstill said 0.5.0. Merging first means the guards are validated against main'scopy_datasetrewrite, which they are..bench/,results.jsonandsummary.mdgitignored — the benchmark runner's defaults write into the repo root.Verification
ciReview notes
mainto publish results. A tag build is on a detached HEAD, so it commits as the bot with[skip ci]and rebase-retries on a race. This is the one outward-facing write added here.benchmarks/andtests/stay out of the wheel; scanpy never entersuv.lockor the Docker image — baselines run from venvs built at benchmark time.🤖 Generated with Claude Code