Conversation
…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>
…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>
Fix the concat --merge hang, and close the gap that hid it
Closes #13, which asked to change the dtype of X/data and X/indices on disk because count matrices stored as float64 waste disk and read time. Generalised to any matrix, plus the CSR<->CSC conversion that concat has been telling people to do since it was written, with nothing to do it. adata convert data.h5ad X -o out.h5ad --dtype float32 adata convert data.h5ad X -o out.h5ad --layout csc adata convert data.h5ad --all -o out.h5ad --dtype float32 Two refusals, both before the destination store exists. A cast is checked by casting every value and casting it back, not by comparing dtypes: whether float64 counts survive float32 depends on the counts, which is the actual question #13 is asking, and the answer is worth printing even when it is yes. A densification is checked against the projected size and refused above 4x growth. --force overrides either and both messages say what was measured. Correctness is held to scipy: every path is compared against what scipy would have produced from the same matrix -- indptr, indices and data each compared exactly, not approximately -- for both transpose directions, both implementations, and across bucket counts from one to hundreds. CSR->CSC->CSR is asserted to be the identity. Transposing streams by default. It cannot be done in one pass, so: count nonzeros per output major to build indptr, scatter the input into buckets by where it lands, then sort and write each bucket in order. Two extra passes over nnz buys a peak set by the chunk rather than by the matrix. --in-memory does it with one lexsort for when the matrix fits. Three things this got wrong on the way, all caught by measuring rather than by reading the code: The first version made files eight times *larger* on a conversion asked for to make them smaller. _growable creates with a fixed 65,536-element chunk, so a 2,400-nonzero output allocated 786 KB of mostly empty chunk; and the index dtype defaulted to int64, silently widening every int32 store that passed through. Outputs are now sized up front -- nnz is known, and is invariant under a transpose -- with chunking and compression forwarded from the source, and the index dtype is preserved unless asked. A test asserts the file shrinks, because that is the feature. The docstring claimed nothing is written before the checks and that was false: the destination was created, and obs and var copied into it, before convert_matrix ran its first check. plan_conversion now resolves and checks every target before the output store is opened, and two tests assert no output file exists after a refusal. The streaming transpose held the whole matrix below 4M nonzeros, because the bucket size was a constant rather than derived from the chunk, so everything smaller went into a single bucket. The performance guard caught it: peak grew 15.9x for 32x the nonzeros on a path whose whole justification is that it does not. Buckets now follow `chunk`. concat's check is unchanged in behaviour but now names the command to run, and is tested for the first time -- CSR+CSC, CSR+dense, CSC+dense, X and layers, including a test that follows the suggested fix and concatenates successfully afterwards. The unreachable duplicate raise is kept and labelled a backstop. Guards: reads linear in nnz for a cast and in n_obs for a layout change; streaming transpose peak bounded while the in-memory one grows, compared in the same run; and an assertion that the default really is the streaming path, since the two produce identical output and nothing else can tell them apart. Benchmark cases convert-dtype and convert-layout, the latter running both implementations so the trade is visible. 952 tests pass, coverage 92.65%, 204 compatibility tests across six anndata releases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The command list and the worked examples both stopped at concat. The new section is its own, not folded into "Filtering without a name list", because shrinking a store and making encodings agree is not filtering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All five reproduced before fixing and re-measured after. Two would have corrupted or destroyed data while reporting success, which is the worst shape a bug can take in something that rewrites people's matrices. An output path naming the input destroyed the store. HDF5 happens to refuse the second open; Zarr does not, so `-o` pointing at the input completed "successfully" and left a store with zero nonzeros where the data had been. Resolved paths are now compared, symlinks and `..` included, and the error points at --inplace, which writes to a temporary file and swaps. An explicitly named matrix outside layers and raw was silently not converted. `_write` recursed into those two groups only, so `obsm/X_pca` -- a path the docs advertise -- was planned, validated, then copied over unchanged with a success message. It now descends into every parent that has a target. Densifying dropped duplicate coordinates instead of summing them. Repeated indices within a major axis are legal in a CSR store and mean their sum, which is what scipy's own `toarray` gives; plain advanced assignment keeps whichever came last. np.add.at accumulates. Reaching this takes a hand-built file, since anndata canonicalises on write -- my first attempt to reproduce it failed for exactly that reason and looked like a false positive. indptr inherited the dtype of indices. A narrow matrix with more than 2^31 nonzeros needs int64 offsets over int32 coordinates; inferring one from the other narrowed the offsets and corrupted the matrix. The two widths are now tracked separately, each preserved from the source, and both range-checked -- always, not only when --indices-dtype is passed, since an inferred dtype can be too narrow just as easily. Transpose buckets were split by coordinate range, not by nonzero count. A skewed matrix therefore landed in one bucket, so the bucket rather than the chunk set the peak and the streaming path could still exhaust memory. Measured on a matrix with 95% of entries in the first five columns, the largest of three equal-width buckets held 88% of them. Bounds now come from the cumulative counts already computed in pass 1, and an explicit `bucket_entries` is honoured as given rather than floored, which is both more correct and what let the test exercise it. Each finding has a test that fails against the previous code. 961 tests pass, coverage 92.98%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add `adata convert`: matrix dtype, layout and density on disk
Adds `adata convert`, and fixes four quadratic paths that made `concat` and `split` appear to hang on real data. Three of the four were found by the new complexity guards rather than by a user; the first was reported from a pipeline that killed twelve tasks after 98 minutes. A minor rather than a patch: `convert` is a new command, and the benchmark and guards are new machinery. Nothing in the existing surface changed behaviour, except that `--merge drop` is now accepted where it was previously rejected despite being the documented default. Also folds the two `### Added` sections that had accumulated under Unreleased into one, and moves the two fixes that had been filed under Added into Fixed. 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. |
Contributor
![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: 63349f520a
ℹ️ 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
+130
to
+131
| for start in range(0, n, chunk): | ||
| block = np.asarray(dataset[start : min(start + chunk, n)]) |
There was a problem hiding this comment.
When --dtype is used on a dense matrix, chunk is treated as a number of rows rather than values. With the default 1,048,576, a typical 100,000 × 30,000 float64 matrix is read completely into memory during preflight, followed by equally large cast and round-trip arrays, so the advertised streaming conversion will likely OOM before creating the output. The CLI's --chunk option does not mitigate this because it only supplies chunk_rows; compute the row step from the width or iterate by a bounded number of elements.
Useful? React with 👍 / 👎.
| convert_matrix( | ||
| src[key], dst, key, plan=by_parent[""][key], **options | ||
| ) | ||
| elif key in by_parent: |
There was a problem hiding this comment.
For a valid target deeper than one group, such as uns/nested/arr, by_parent contains uns/nested, but this root-level test only checks for the key uns. The entire subtree therefore takes the copy_tree branch, and the command reports success while leaving the requested matrix unchanged. Detect descendant prefixes and recurse through groups until the planned parent is reached.
Useful? React with 👍 / 👎.
Comment on lines
+163
to
+166
| if file.is_dir(): | ||
| shutil.rmtree(file) | ||
| elif file.exists(): | ||
| file.unlink() |
There was a problem hiding this comment.
During --inplace, the completed temporary store is not swapped atomically: the original file or directory is deleted before move/replace is attempted. If that operation fails or the process is interrupted in this window, the source path is missing, and a Zarr rmtree failure can even leave a partially deleted store, contradicting the command's stated safety guarantee. Keep the original until a replacement succeeds, using os.replace directly for files and a backup/rollback sequence for directories.
Useful? React with 👍 / 👎.
Contributor
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.
Release 0.6.0. Brings
devtomain: 17 commits across two merged PRs, #14 and #15.Added
adata convert— change a matrix's dtype, layout (CSR/CSC/dense) or density on disk, streaming. Closes #13: counts stored as float64 halve with--dtype float32. Refuses a lossy cast or a >4× size increase before writing anything.tests/test_performance.py) covering every subcommand. They count operations rather than seconds, so nothing fails because a runner was busy.viewandlsare now held to reading zero data elements at any store size.benchmarks/) against anndata and scanpy, published to docs/BENCHMARKS.md on every tag. Report-only.--merge drop/--uns-merge dropare accepted;dropwas the documented default but rejected as a value.Fixed — four quadratic paths
The first was reported from a pipeline that killed twelve tasks after 98 minutes. The other three were found by the new guards, not by users.
concat --mergere-read a var column per variableconcatcategory union used list membershipsplit --byrescanned each chunk per labelconcatbuilt a Python object per obs rowPlus two found by automated review and confirmed by measurement:
copy_datasetignored its own read budget for variable-length strings (827 MB against a stated 32 MiB), and the benchmark's own peak-RSS measurement was floored by the runner's memory on Linux.Verification
pyproject.tomlis 0.6.0, matching the tagcheck-versionwill compare againstAfter merge
Tagging
0.6.0fires three workflows:publish.yml(PyPI),quay-on-tag.yml(image) and the newbenchmark.yml, which is report-only and commits its results todocs/.🤖 Generated with Claude Code