diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25e9507..db21240 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,8 +10,10 @@ name: CI # tests/test_cell_stream.py, tests/test_inmem_external_ref_gpu.py) each # carry a module-level `pytest.importorskip("cellstream")`, so all three files # skip entirely without the optional `[streaming]` extra. Between them that -# is 141 cases, 70 of which need no GPU -- and since cellstream is on PyPI -# those 70 are now runnable on a stock runner, which wants its own job. +# is 147 cases, 76 of which need no GPU (measured 2026-08-20; per suite, +# test_shard_stream 32/37, test_cell_stream 44/8, test_inmem_external_ref_gpu +# 0/26) -- and since cellstream is on PyPI those 76 are now runnable on a +# stock runner, which wants its own job. # - The real-data tests (tests/test_real_data.py) skip when the dataset path # is absent (it always is on CI). # So this job goes green on a stock CPU runner with no GPU and without the @@ -71,17 +73,16 @@ jobs: cache-dependency-glob: pyproject.toml # `uv pip install` does pip-style resolution of only the requested - # extras. It used to be mandatory: a fresh `uv sync` resolves EVERY source - # in [tool.uv.sources], and that table carried a private cellstream (then - # shardad) git-SSH entry the no-SSH runner could not clone, even when the - # `streaming` extra wasn't requested. That entry is gone since the rename, - # so `uv sync` would work now too. torch resolves from PyPI here (the cu126 + # extras. It used to be mandatory, back when [tool.uv.sources] carried a + # git-SSH entry a no-SSH runner could not clone and `uv sync` resolved every + # source in that table whether or not the extra was requested. That entry is + # gone, so `uv sync` would work now too. torch resolves from PyPI here (the cu126 # source is skipped), which is fine: CI is CPU-only and GPU tests skip. # `[fast]` (numba) IS installed so the numba CSR kernel — a CPU code path — # is exercised rather than the scipy fallback. # # `[streaming]` is deliberately NOT installed here: this job's purpose is - # to prove gpudge's CORE install needs nothing optional. Covering the 70 + # to prove gpudge's CORE install needs nothing optional. Covering the 76 # non-CUDA cellstream cases is now possible on a stock runner and wants its # own job rather than widening this one. - name: Install dependencies diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8581e5d..db85e40 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -57,7 +57,7 @@ jobs: set -euo pipefail expected=$(printf '%s\n' \ .gemini .github .gitignore .python-version \ - CHANGELOG.md CITATION.cff CODE_OF_CONDUCT.md CONTRIBUTING.md \ + AUTHORS CHANGELOG.md CITATION.cff CODE_OF_CONDUCT.md CONTRIBUTING.md \ LICENSE PKG-INFO README.md SECURITY.md \ benchmarks docs environment.yml examples pyproject.toml src tests \ | sort) @@ -102,4 +102,185 @@ jobs: echo "::error::tag $GITHUB_REF_NAME does not match built version $built"; exit 1; } echo "tag and built version agree: $built" + # Four files carry the release version independently of pyproject.toml -- + # CITATION.cff, CHANGELOG.md, environment.yml, README.md -- and nothing + # used to check any of them: CITATION.cff silently shipped `0.8.0` INSIDE + # the 0.9.0 release, because "these files move together" lived only in a + # human's head. Asserted here instead. + # + # The two YAML files are PARSED and the two Markdown files are matched as + # whole lines. Both decisions are findings rather than preferences: three + # rounds of review produced file contents that passed a text-matching + # draft of this step while publishing the wrong thing. + # + # grep -F matched a version named in a COMMENT while the real pin + # sat one release behind. + # grep -c counts matching LINES, so an exact heading string buried + # in a sentence of prose read as the heading. + # digits a date checked for SHAPE let `2026-02-31` through, and + # `fromisoformat` alone lets `20260820` through -- it + # canonicalises, and CFF 1.2 wants the dashes. + # ERE interpolating a version into a regex breaks on the `+` + # that PEP 440 permits. + # .strip() made the checks indentation-blind, so a pin de-indented + # out of the `pip:` block (where it installs nothing) and a + # heading indented into a code block both passed. + # startswith a duplicate `"version":` key, which YAML resolves to the + # LAST value, hid behind the first one being right. + # substring counting occurrences of the install pin accepted one + # inside an HTML comment, with none left in the document. + # truthiness an emptied or fully-commented-out YAML file parses to + # None, and a bare `{}` parses to an empty mapping; both are + # falsy, and both skipped their structural check entirely. + # `.*?-->` an HTML comment opener with the closer forgotten hides the + # rest of the document when rendered and stripped nothing. + # + # Every one of those was demonstrated against this step. (Codex review, + # gpudge_arc#149.) + - name: The other files that carry the version agree with it + run: | + set -euo pipefail + uv run --with pyyaml --no-project python - <<'PY' + import glob, re, sys + from datetime import date + from pathlib import Path + + import yaml + + fail = [] + + class Strict(yaml.SafeLoader): + """Rejects duplicate keys, which SafeLoader resolves to the last.""" + + def _no_dupes(loader, node, deep=False): + seen = set() + for k, _ in node.value: + key = loader.construct_object(k, deep=deep) + if key in seen: + raise yaml.YAMLError(f"duplicate key {key!r}") + seen.add(key) + return yaml.SafeLoader.construct_mapping(loader, node, deep) + + Strict.add_constructor( + yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, _no_dupes) + + def load(name): + """A mapping, or None with the reason already recorded. + + Everything that is not a mapping is a failure, `None` included: an + emptied or fully-commented-out file parses to None, and letting + that through skipped the whole structural check below. Callers + test `is not None`, not truthiness -- `{}` is a mapping, parses + from a plain `{}`, and is falsy, which skipped the check just the + same. + """ + try: + doc = yaml.load(Path(name).read_text(encoding="utf-8"), Strict) + except yaml.YAMLError as exc: + fail.append(f"{name} is not valid YAML: {exc}") + return None + if not isinstance(doc, dict): + fail.append(f"{name} did not parse to a mapping, got {type(doc).__name__}") + return None + return doc + + def lines(name): + return Path(name).read_text(encoding="utf-8").splitlines() + + sdists = sorted(glob.glob("dist/*.tar.gz")) + if len(sdists) != 1: + print(f"::error::want exactly one dist/*.tar.gz, found {len(sdists)}: {sdists}") + sys.exit(1) + stem = Path(sdists[0]).name.removesuffix(".tar.gz") + if not stem.startswith("gpudge-"): + print(f"::error::cannot read a version out of {sdists[0]}") + sys.exit(1) + v = stem[len("gpudge-"):] + + # CITATION.cff, as a citation consumer sees it. `date-released` is a + # date in CFF, so an unquoted value parses to a date object; a quoted + # one must be the canonical YYYY-MM-DD, which is what CFF 1.2 asks for + # and is stricter than `fromisoformat`, whose 3.11+ form also accepts + # `20260820` and `2026-W34-4` and quietly canonicalises them. + cff, cff_date = load("CITATION.cff"), None + if cff is not None: + if str(cff.get("version")) != v: + fail.append(f"CITATION.cff version is {cff.get('version')!r}, want {v}") + raw = cff.get("date-released") + if isinstance(raw, date): + cff_date = raw.isoformat() + else: + try: + parsed = date.fromisoformat(str(raw)).isoformat() + except (ValueError, TypeError) as exc: + fail.append(f"CITATION.cff date-released {raw!r} is not a real date: {exc}") + else: + if parsed != str(raw): + fail.append(f"CITATION.cff date-released {raw!r} is not written as {parsed}") + else: + cff_date = parsed + + # environment.yml: the pin has to be in the PIP list. Checked + # structurally because a pin de-indented into the conda list is still + # a well-formed list entry -- it just never installs gpudge. + env = load("environment.yml") + want = f"gpudge[fast]=={v}" + if env is not None: + pips = [item["pip"] for item in env.get("dependencies") or [] + if isinstance(item, dict) and "pip" in item] + if len(pips) != 1: + fail.append(f"environment.yml has {len(pips)} pip: blocks, want exactly 1") + elif [r for r in pips[0] if r == want] != [want]: + fail.append(f"environment.yml pip: block does not carry exactly one {want!r}: {pips[0]}") + + # And no OTHER version pinned anywhere in either file, including in a + # comment a parser cannot see. Every occurrence is extracted and + # compared: deciding per LINE pardoned a stale pin sitting in a + # trailing comment beside the right one, and a substring test let + # `==0.9.10` satisfy a check for `==0.9.1`. + for name in ("environment.yml", "README.md"): + found = re.findall(r"gpudge\[fast\]==([0-9A-Za-z._!+-]*)", + Path(name).read_text(encoding="utf-8")) + other = sorted({f for f in found if f != v}) + if other: + fail.append(f"{name} pins gpudge[fast] at {other} as well as {v}") + + # The changelog heading, whole-line and UNINDENTED: `.strip()` accepts + # four leading spaces, which CommonMark renders as a code block, so the + # release's sections would stay under Unreleased. + if cff_date: + want_h = f"## [{v}] \u2014 {cff_date}" + n = sum(1 for ln in lines("CHANGELOG.md") if ln == want_h) + if n != 1: + fail.append(f"want exactly one unindented CHANGELOG heading {want_h!r}, found {n}") + + # README's install pin is the one version in this tree NOT edited by + # hand: the mirror build substitutes it from a version argument typed + # on the command line, so it can name a different tag while every + # hand-edited file agrees -- and it is the line readers copy. HTML + # comments are stripped first: the pin has to be in the document a + # reader sees, and commenting the install block out (delimiters on + # their own lines, pin untouched) satisfied a plain line match. An + # opener runs to `-->` OR to end of file, because that is what + # CommonMark does with an unclosed one -- and `.*?-->` strips nothing + # at all when the closer was forgotten. + text = re.sub(r"|\Z)", "", + Path("README.md").read_text(encoding="utf-8"), flags=re.S) + want_pin = f'"gpudge[fast] @ git+https://github.com/ArcInstitute/gpudge.git@v{v}"' + n = sum(1 for ln in text.splitlines() if ln.strip() == want_pin) + total = text.count("ArcInstitute/gpudge.git@") + if n != 1 or total != 1: + fail.append( + f"README.md: want exactly one uncommented line {want_pin} and no " + f"other 'ArcInstitute/gpudge.git@'; found {n} and {total}. The " + f"mirror build's version argument disagrees with the tag") + + for f in fail: + print(f"::error::{f}") + if fail: + sys.exit(1) + print(f"version parity ok: {v}, dated {cff_date}, in CITATION.cff, " + f"CHANGELOG.md, environment.yml and README.md") + PY + - uses: pypa/gh-action-pypi-publish@release/v1 diff --git a/.gitignore b/.gitignore index 9c413cc..f9da33c 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ htmlcov/ uv.lock + diff --git a/AUTHORS b/AUTHORS new file mode 100644 index 0000000..e7a869d --- /dev/null +++ b/AUTHORS @@ -0,0 +1,83 @@ +gpudge — authors and contributors +================================= + +Maintainer +---------- + +Alexander Dobin (@alexdobin) + Author and maintainer. + + +Inspiration and prior art +------------------------- + +Abhinav Adduri (@abhinadduri) + gpudge's direct inspiration is the GPU fork of pdex — [`abhinadduri/pdex`, branch + `gpu`](https://github.com/abhinadduri/pdex/tree/gpu), where + [`138335ae`](https://github.com/abhinadduri/pdex/commit/138335ae97b8) "Add CUDA backend for + ref-mode MWU" added `src/pdex/_torch_mwu.py` on 2026-05-19. gpudge's own design document is + dated two days later. That branch put reference-mode Mann–Whitney on CUDA — sorting the + fixed reference pool once per gene chunk and reusing it across every target group — and + added `mwu_backend={"auto","cpu","torch"}` and a `gpu_gene_chunk_size` knob to + `pdex.pdex(...)` to drive it. As of 2026-08-20 those interfaces are the fork's alone: + upstream pdex has no `_torch_mwu.py`, and no `mwu_backend`, `gpu_gene_chunk_size` or + `default_gene_chunk_size`. + + gpudge took two concrete things from that branch as well as the idea. The name of the + `gpu_gene_chunk_size` parameter is that branch's, and so is the shape of gpudge's gene-chunk + sizer: a working-set estimate of ~24 bytes per reference cell per gene, capped at 16 GB and + at a fraction of free device memory, floored and rounded down to a multiple of 64. gpudge + takes 20% of free memory where the fork takes 18%, and additionally accounts for the + per-chunk accumulators the reference path holds resident — but it is the same heuristic, + adopted rather than reinvented. The attribution comment at that call site said plain "pdex", + which read as upstream; it now names the fork. + + Thanks also for a number of useful suggestions along the way. + +gpudge exists to replace the parts of [pdex](https://github.com/ArcInstitute/pdex) that one +production DE stage used, and pdex is the CPU baseline behind gpudge's parity and speedup +claims. It is prior art in the strong sense: those numbers only mean anything because there +was a correct, well-tested CPU implementation to measure against. (What the committed +real-data check asserts is a bounded agreement, not bit-identity — see the tolerances in +`tests/test_real_data.py`.) + + +Production evaluation +--------------------- + +Kevin Feng (@kfenggg) + Evaluated gpudge in a production single-cell QC pipeline and adopted it as that pipeline's + default differential-expression engine, reporting close log2-fold-change agreement with CPU + pdex on real data at production scale, and a large reduction in wall time against the CPU + path it replaced. An evaluation run by someone other than this library's author, on data + and a workload chosen without reference to gpudge's own test suite, is a different kind of + evidence from the in-repo benchmarks — complementary to them, not a replacement. + + Establishing that agreement meant working out the correspondence between the two engines, + which is his. On log1p input, pdex's default target and reference means are, in exact + arithmetic, `expm1` of gpudge's ARITHMETIC means — and log2FC then has to be recomputed + from those transformed means, because gpudge's arithmetic-mode `log2_fold_change` is a + ratio of the untransformed means and is not on pdex's scale. Arithmetic is the mode to use + here because of a real gpudge constraint: gpudge refuses `mean_calc="geometric"` together + with `ALL_OTHERS`, the rest-mean being computed by sum-subtraction, which is implemented + for the arithmetic case only. + + +AI tooling +---------- + +Much of this codebase was written with Claude Code (Anthropic), and OpenAI Codex was used inside +those same sessions to review the work before it reached human review — a deliberate +cross-provider check, on the theory that a second model from a second vendor is harder to talk +into agreement than the one that wrote the code. Gemini Code Assist and GitHub Copilot review +every pull request on top of that. The development history carries `Co-Authored-By` trailers +naming the specific Claude models. + +Design decisions, rulings and acceptance of every change are the maintainer's. + + +Copyright +--------- + +Copyright (c) 2026 Arc Research Institute. +MIT — see LICENSE. diff --git a/CHANGELOG.md b/CHANGELOG.md index d424ca3..cbe3ea3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to gpudge are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +> Entries before 0.9.0 call the optional streaming dependency **`shardad`**. That +> is the same package now published as +> [`cellstream`](https://github.com/ArcInstitute/cellstream); it was renamed when +> it went to PyPI in 0.9.0. Those entries keep the name they were written with +> rather than being rewritten. + > **A note on issue and PR numbers.** References such as `#124`, `gpudge_arc#115` > and `shardad #246` throughout this file are **provenance, not links**: they > identify entries in issue trackers that are not public, and will not resolve @@ -13,6 +19,167 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.9.1] — 2026-08-20 + +### Changed + +- **`de(archive=…, layout='cell')` no longer reaches into cellstream's private + `CellStore._load_groups()`.** It reads the public `group_spans()` that + cellstream 0.8.0 added for exactly this (`shardad #248`), plus the manifest's + `reference` — which loading the spans validates against groups.json's own copy, + so the two cannot disagree. Behaviour is unchanged: same group table, same + guards, same messages apart from the one that named the accessor. Verified on + real archives in both reference modes that `group_spans()` returns the private + records element-for-element and that the manifest's `reference` matches + groups.json's. (`gpudge_arc#139`) + + gpudge still **rejects duplicate group labels**, which cellstream explicitly + permits — under the default sort order, distinct raw obs values that are equal + once stringified produce several records carrying one label, which is why + `group_spans()` returns a list rather than a mapping. For a *target* label that + is a wrong answer, and not by collapsing the spans: the target list keeps both + entries while the label→index map resolves both to the last, so one result row + is never written and the other is overwritten span by span. For a *reference* + label the rejection is stricter than necessary — duplicates are handled + correctly downstream — and is left in place deliberately, that path having no + fixture; loosening it is a follow-up, not part of this change. + + `reference_row_count()` is deliberately **not** used. The label split has to + happen regardless, and deriving the reference block's end from the same table + keeps the two consistent by construction rather than by agreement. + +### Added + +- **`AUTHORS`**, a curated credit record. `git shortlog` describes this project + badly — squash merges collapse each pull request into one author entry, a shape + that cannot represent a review, a report or prior art, and the contribution + that matters most here arrived as exactly those rather than as patches. It + credits the GPU fork of pdex (`abhinadduri/pdex@gpu`) as + the work that demonstrated the idea gpudge is built on, and from which gpudge + adopted both the `gpu_gene_chunk_size` parameter name and the shape of its + automatic gene-chunk sizer. The attribution comment at that call site said + "pdex", which read as upstream; upstream pdex has no such function, and the + comment now names the fork. + + It also records a production evaluation by someone other than this library's + author: gpudge was adopted as the default DE engine in a production single-cell + QC pipeline, with close log2-fold-change agreement against CPU pdex reported on + real data at production scale. The entry states the correspondence that makes + the two comparable — on log1p input pdex's means are `expm1` of gpudge's + *arithmetic* means, and log2FC must be recomputed from those, because + arithmetic-mode `log2_fold_change` is a ratio of the untransformed means. + + A `.mailmap` accompanies it so identity counts over the private history are + correct. It is development-only and is not part of a release. + +### Removed + +- **`docs/THIRD_PARTY_LICENSES.md`.** It was a dated audit of one resolved + dependency environment, and gpudge **redistributes** no third-party code — the + wheel carries only the `gpudge/` package, which the release workflow asserts on + every publish — so there was no notice obligation for the file to discharge. + What it did instead was restate, less authoritatively, what each dependency + declares in its own metadata. And not every package declares anything: + `pyfastpfor`, a transitive dependency of the streaming extra's `[cell]` variant, + publishes an sdist with no licence file at all, which is the kind of row a + point-in-time table records once and then cannot keep true. It had also gone materially + stale: its rows described the streaming dependency under a name and a version + that no longer exist, and refreshing them meant reproducing a 93-package + point-in-time resolve rather than editing text. `SECURITY.md` now states the + actual situation instead of linking to it. + + The mirror pipeline loses two things with it: a fixup rule that de-sited the + audit's regeneration recipe, and — more usefully — the last exception in + `leak_check.py`'s SSH-URL check, which was scoped to that one file. A GitHub + SSH URL anywhere in the published tree is now a leak with no exception at all — + as this very entry discovered, having first been written with a literal one in + it. + +### Fixed + +- **`de(archive=…)`'s missing-dependency error gave dated install advice.** It + told the caller to run `pip install '.[streaming]'` *from a checkout*, because + the bare PyPI name still resolved to the reservation stub. gpudge is on PyPI + as of 0.9.0, so the message now leads with `pip install 'gpudge[streaming]'` + and keeps the checkout form as the alternative. Deferred from the 0.9.0 + release review rather than re-cutting the tag. + +- **Two install hints were unquoted.** `pip install gpudge[fast]`, as printed by + the no-numba warning and by `_csr_dense`'s module docstring, is a glob pattern + to zsh — the default shell on macOS — which fails with `no matches found` + rather than installing anything. Both now quote the argument. + +- **The pre-release GPU gate now reports what it actually measured.** Its first + run ever to reach the test suite passed every case on an unpinned `[dev,fast]` + resolve on an L4 and still reported failure, on two faults in the gate's own + harness rather than anything in the library: a correct new skip its verdict + allow-list had not been told about, and one artifact copy that failed + transiently with the reason discarded. Reviewing that fix surfaced two further + faults that had never been observed — the retrieval loop leaked a non-zero + status, and its error-reporting path could end the run at SIGPIPE before the + verdict or the diagnostics were written. All four are fixed and the loop now + has unit tests driven against the gate implementation itself rather than a + copy of it. + + This is the machinery that decides whether a release may happen; the gate + implementation itself is deliberately not part of any distribution. + (`gpudge_arc#147`) + +- **The streaming extra's missing-dependency error no longer names a package that + no longer exists.** It read "installs cellstream (>=0.9.0, the former + shardad)"; the parenthetical helped nobody who had hit the error, and named a + repository the reader cannot see. The same aside appeared across the prose — the + install documentation, two packaging comments and a CI comment each explained a + pre-rename `[tool.uv.sources]` git-SSH entry that broke `uv sync`, a failure + impossible on any supported version now that the floor is cellstream 0.9.0 and + that entry is gone. All removed. + +- **`CITATION.cff` said `0.8.0` while 0.9.0 was being published**, so anyone who + cited this library from its own metadata cited the wrong release. Corrected, + and the publishing workflow now asserts it rather than trusting that "these + files move together": four files carry the release version independently of + `pyproject.toml`, and nothing checked any of them. + + The two YAML files are **parsed** — with a loader that rejects duplicate keys, + and a structural search for the pin inside the `pip:` block — and the two + Markdown files are matched as whole lines, the changelog heading unindented + and the README pin after HTML comments are stripped out. Those decisions are + findings rather than preferences. Three rounds of review produced file + contents that passed a text-matching draft of this check while publishing the + wrong thing: + + - a version named in a *comment* while the real pin sat one release behind, + and a stale pin in a trailing comment beside the correct one; + - the install pin inside an *HTML comment*, with none left in the document a + reader sees — and then the same pin under an *unclosed* comment opener, + which hides the rest of the page when rendered while `` strips + nothing at all; + - an exact heading string buried inside a sentence of prose, because `grep -c` + counts matching *lines*; + - a heading indented four spaces, which renders as a code block, so the + release's sections stay under `Unreleased`; + - a date checked for digit shape, so `2026-02-31` passed — and, once parsing + replaced the shape check, `20260820`, because `date.fromisoformat` accepts + it and canonicalises it while CFF wants the dashes; + - a duplicate `"version":` key, which YAML resolves to the *last* value, + hidden behind a first one that was right; + - the pin de-indented out of the `pip:` block, where it is still a well-formed + list entry and installs nothing; + - an emptied, fully-commented-out file, which parses to `None`, and a bare + `{}`, which parses to an empty mapping — both falsy, both skipping their + structural check entirely, and the second introduced by the fix for the + first; + - `==0.9.10` satisfying a check for `==0.9.1`, twice, in two different checks. + + Interpolating a version into a regular expression fails on a different axis — + the `+` that PEP 440 permits. Every one of those was demonstrated against this + check rather than argued about, and each is now a case that fails. + + `README.md` is checked because it holds the install pin readers copy, and in + the published tree that version is the one thing *not* hand-edited: the mirror + build substitutes it from a version argument typed on the command line, so it + can name a different tag while every hand-edited file agrees. (`gpudge_arc#149`) + ## [0.9.0] — 2026-08-19 ### Changed @@ -905,8 +1072,8 @@ Distribution remains **Arc-internal** — install from the `v0.3.0` git tag - First CI workflow (`.github/workflows/ci.yml`): `ruff` + CPU `pytest` on every push / PR (ubuntu-latest, Python 3.12). Installs via - `uv pip install -e ".[dev,fast]"` so the private shardad git-SSH source is - never resolved on the runner; GPU (`needs_cuda`), shardad streaming + `uv pip install -e ".[dev,fast]"` so the streaming dependency's git-SSH source + is never resolved on the runner; GPU (`needs_cuda`), streaming (`importorskip`), and real-data tests auto-skip. The `ruff` gate is blocking; the repo was brought to zero lint errors (47 fixed). Green at 121 passed / 53 skipped. (#53) @@ -1089,11 +1256,12 @@ ULP of the CPU baseline on every assertable column. until shardad v0.3 lands an on-GPU read path. -[Unreleased]: https://github.com/ArcInstitute/gpudge/compare/v0.9.0...HEAD +[Unreleased]: https://github.com/ArcInstitute/gpudge/compare/v0.9.1...HEAD +[0.9.1]: https://github.com/ArcInstitute/gpudge/compare/v0.9.0...v0.9.1 [0.9.0]: https://github.com/ArcInstitute/gpudge/compare/v0.8.0...v0.9.0 [0.8.0]: https://github.com/ArcInstitute/gpudge/compare/v0.7.0...v0.8.0 [0.7.0]: https://github.com/ArcInstitute/gpudge/compare/v0.3.1...v0.7.0 diff --git a/CITATION.cff b/CITATION.cff index 0e21239..4197678 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -13,8 +13,8 @@ authors: - family-names: Dobin given-names: Alexander affiliation: "Arc Institute" -version: 0.8.0 -date-released: "2026-08-18" +version: 0.9.1 +date-released: "2026-08-20" license: MIT repository-code: "https://github.com/ArcInstitute/gpudge" keywords: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 09cc362..1cb25e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,10 +28,7 @@ uv venv && uv pip install --torch-backend=cu126 -e ".[dev,fast]" `uv pip install` resolves only the extras you name, and skips `[tool.uv.sources]` — which is why `--torch-backend=cu126` is passed explicitly. -`uv sync` also works now; it did not before the `shardad` → `cellstream` rename, -because `[tool.uv.sources]` then carried a private git-SSH entry that `uv sync` -resolved even when the `streaming` extra was not requested, failing with -`Permission denied (publickey)` for anyone without a key. `[fast]` adds numba, +`uv sync` also works. `[fast]` adds numba, which the fast CSR kernel needs; `[dev]` adds pytest, ruff, scanpy and pyyaml. ## Running the tests @@ -93,6 +90,10 @@ thing to write, and much better than silence. If a reviewer's suggestion looks wrong to you, say why rather than applying it; several suggestions in this repository's history were technically reasoned and empirically false. +Add yourself to `AUTHORS` in the same PR. That applies to a report, a +measurement or a review that changed the code or a documented rule, not only to +a patch — `AUTHORS` says so, and does not rank the two. + ## Reporting a bug Include the gpudge version, the Python and torch versions, the GPU, and the diff --git a/README.md b/README.md index 3e00ed5..23eedeb 100644 --- a/README.md +++ b/README.md @@ -122,7 +122,7 @@ not been released yet: ```bash pip install --extra-index-url https://download.pytorch.org/whl/cu126 \ - "gpudge[fast] @ git+https://github.com/ArcInstitute/gpudge.git@v0.9.0" + "gpudge[fast] @ git+https://github.com/ArcInstitute/gpudge.git@v0.9.1" ``` Extras: @@ -172,11 +172,8 @@ uv venv && uv pip install --torch-backend=cu126 -e ".[dev,fast]" deliberately — it is CPU-only. `uv sync --extra streaming` also works and pulls the cu126 torch build from its -pin with no manual `--extra-index-url`. (Before the `shardad` → `cellstream` -rename this was not true: `[tool.uv.sources]` carried a private git-SSH entry -that `uv sync` resolved even when the `streaming` extra was not requested, so it -failed with `Permission denied (publickey)` for anyone without a key. That entry -is gone — cellstream comes from PyPI.) +pin with no manual `--extra-index-url`. cellstream resolves from PyPI like any +other dependency, so no repository access is involved. ### conda / mamba diff --git a/SECURITY.md b/SECURITY.md index c88c512..6095846 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -44,7 +44,10 @@ run arbitrary code in the same process. ## Dependencies -Third-party licences and the environment a release was audited against are -recorded in [`docs/THIRD_PARTY_LICENSES.md`](docs/THIRD_PARTY_LICENSES.md). A +gpudge **redistributes** no third-party code: the wheel carries only the +`gpudge/` package and its `dist-info`, which the release workflow asserts on +every publish. Dependencies are resolved at install time from the set declared +in `pyproject.toml`, and each carries its own licence — in its own metadata +where it declares one at all, which not every package on PyPI does. A vulnerability in a dependency is best reported upstream first; tell us too, so the floor can be raised here. diff --git a/docs/THIRD_PARTY_LICENSES.md b/docs/THIRD_PARTY_LICENSES.md deleted file mode 100644 index f6cce84..0000000 --- a/docs/THIRD_PARTY_LICENSES.md +++ /dev/null @@ -1,394 +0,0 @@ -# Third-party licenses - -License audit of the packages `gpudge` depends on. **`gpudge` itself is -licensed `MIT`** (see `LICENSE` and `pyproject.toml`). - -> **Audited 2026-08-02** against a **recorded point-in-time clean environment -> resolved from the `v0.7.0` tree** (CPython 3.12.11, uv 0.7.12, the `cu126` -> PyTorch build, `dev` + `fast` + `streaming-gpu` extras; 93 distributions). The -> recipe below is the method — use it for the next audit; it is not guaranteed to -> reproduce these exact versions, and is not meant to. -> -> **These versions are a point-in-time resolve, not committed project state.** -> gpudge declares floors (`anndata>=0.12`, `torch>=2.5`, …) against a mutable -> index, so only `shardad` — tag-pinned via `[tool.uv.sources]` — is fixed by the -> repository; everything else is what the resolver returned on 2026-08-02. The -> `uv.lock` generated by that run — `$AUDIT/tree/uv.lock` in the recipe below — -> has SHA-256 -> `dd5a4aa115eb0a9ddaca8a1b83024745452cd13328e2258959f8f04fa15e01d8`. Re-audit -> after dependency bumps. -> -> **Not** the developer `.venv`: that environment installs `shardad` from a local -> working checkout (`direct_url.json` → `file:///…/shardad`, version -> `0.7.2.dev24+…`), which is not evidence for any released version, and it carries -> unrelated editable installs that are outside gpudge's dependency closure. -> -> **Package-level** licenses were read from installed distribution metadata, not -> assumed. Where a package carries no **structured license identifier** — no -> `License-Expression`, no legacy `License` value, no `License ::` classifier — -> the `LICENSE` file shipped in the distribution (referenced by `License-File:`) -> is cited instead and called out as such. Where the *only* identifier is the -> coarse legacy `BSD License` classifier (SciPy, numba), the precise SPDX -> identifier is resolved from the shipped license text and marked "from -> `LICENSE`" — metadata alone cannot distinguish 2-clause from 3-clause. Where a -> package has **neither** — no identifier *nor* any license file in any published -> artifact (`pyfastpfor`) — the grant is taken from the license headers carried in -> that release's own source, cited by file, and flagged as an artifact-level gap -> rather than recorded as a clean license. Claims about *bundled components* whose -> license information the wheel does not itself carry (notably hdf5plugin's HDF5 -> filters — the wheel ships the eleven filter `.so` binaries, but nothing stating -> their terms) are attributed to upstream and flagged as such. -> -> Supersedes the 2026-06-09 audit, which predated the MIT relicense (#60) and -> therefore described gpudge as BSD-3-Clause throughout. -> -> Supersedes the 2026-07-25 audit, which was taken against the developer `.venv` -> and therefore recorded a `shardad` version predating the `[cell]` extra entirely, -> and omitted `pyfastpfor` and `pybind11` (`gpudge_arc#120`). -> -> ⚠️ **The `shardad` row below is stale as of `gpudge_arc#139`.** That package has -> been renamed **`cellstream`**, is published on PyPI, and the `streaming` extras -> now pin `cellstream>=0.9.0` — so the private-repo access this recipe -> describes is no longer needed, and `[tool.uv.sources]` no longer carries a -> git-SSH entry for it. Everything below is left exactly as it was measured on -> the date in the banner: this file is a dated record, and the correct fix is to -> re-run the regeneration recipe against the new dependency rather than to edit -> the recorded rows by hand. - -## How to regenerate - -First build the environment. Resolve it from a clean checkout of the tagged tree, -never from a developer `.venv` — this is the step whose absence made the previous -audit ambiguous about which environment it described. - -**This recipe needs access to the private `ArcInstitute/shardad` repository**: -`--extra streaming-gpu` resolves shardad over SSH, so the full 93-package audit -is maintainer-only. Everything the core install pulls can be audited without it -by dropping that extra. - -```bash -set -euo pipefail # without errexit a failed mkdir/tar/cd falls through to - # `uv sync` in whatever directory you started in - -# Put $AUDIT on a large scratch filesystem, never /home: the resolved -# environment is ~8 GB. Substitute your own site's path -- and check that you -# can create a directory there: a shared scratch root is often root-owned 0755, -# in which case $SCRATCH/$USER is NOT creatable and a per-group path is. -SCRATCH_ROOT=/path/to/scratch -AUDIT=$SCRATCH_ROOT/gpudge-license-audit-$(date +%Y%m%d-%H%M%S) -# A fresh, unique directory: `mkdir -p` over a populated path silently overlays -# it, which is exactly the contamination this recipe exists to avoid. -mkdir "$AUDIT" "$AUDIT/tree" -# `-C /path/to/gpudge` is deliberate, not redundant: nothing above this line -# puts you in the gpudge checkout, so a bare `git archive` would depend on an -# unstated cwd. `tar -xf -` names stdin explicitly, which BSD/macOS tar wants. -git -C /path/to/gpudge archive v0.7.0 | tar -xf - -C "$AUDIT/tree" -cd "$AUDIT/tree" -uv sync --python 3.12 --extra dev --extra fast --extra streaming-gpu -``` - -`[tool.uv.sources]` pins shardad to its `v0.7.1` tag, so that sync resolves -`shardad==0.7.1` from -`git+ssh://git@github.com/ArcInstitute/shardad.git@fbba761` — the version this -audit describes, and the one version here the repository actually fixes. Confirm -it before reading anything else: - -```bash -.venv/bin/python -c "import importlib.metadata as M; print(M.version('shardad'))" -# 0.7.1 -``` - -Then dump the closure: - -```bash -.venv/bin/python - <<'PY' # the $AUDIT/tree/.venv from above -import importlib.metadata as M -for d in sorted(M.distributions(), key=lambda d: (d.metadata.get("Name") or "").lower()): - md = d.metadata - name = md.get("Name") - if not name: - continue - lic = (md.get("License-Expression") - or "; ".join(dict.fromkeys( - v.split("::")[-1].strip() for k, v in md.items() - if k == "Classifier" and v.startswith("License ::"))) - or (md.get("License") or "").strip().splitlines()[0:1] or ["(none declared)"]) - print(f"{name}\t{d.version}\t{lic if isinstance(lic, str) else lic[0]}") -PY -``` - -Four caveats when reading that output: - -- It reads only the **identifier fields**. Several packages below leave those - empty yet ship a real `LICENSE` file; check `/licenses/` before - concluding anything is unlicensed. -- It prefers a `License ::` classifier over the legacy `License` value, so for - SciPy it prints just `BSD License` and **never reaches** that package's - ~47 kB `License` value — which is where the bundled binary components and - their SPDX identifiers are declared. Inspect that field separately; see the - SciPy subsection below. -- It reports **whatever is installed**. In the clean environment above that *is* - gpudge's dependency closure for those extras (93 distributions, `gpudge` itself - included). Run it against a developer `.venv` and it is not: that environment - also carries unrelated editable installs, which is one more reason the recipe - above starts from `git archive`. -- A `License ::` classifier can be **present and still name nothing**. `umap-learn` - declares exactly `License :: OSI Approved`; because the snippet keeps only the - segment after the final `::`, that prints as a bare `OSI Approved`. Treat any - output that does not name a license family as "not yet audited". - -## Declared dependencies (`pyproject.toml`) - -| Package | Version | Extra | License | Notes | -|---|---|---|---|---| -| anndata | 0.13.2 | core | BSD-3-Clause | permissive | -| hdf5plugin | 7.0.0 | core | MIT (wrapper only) | **no structured identifier**; taken from the shipped `LICENSE`, which grants MIT for "the hdf5plugin code itself". The bundled HDF5 filters carry **separate** terms (BSD / Zlib / etc.), and neither their notices nor upstream's `doc/information.rst` are included in the installed wheel — see flag 3 | -| numpy | 2.4.6 | core | BSD-3-Clause AND 0BSD AND MIT AND Zlib AND CC0-1.0 | the vendored components are the non-BSD terms — all permissive | -| polars | 1.43.2 | core | MIT | permissive; polars 1.43 splits its compiled runtime into an unconditional `polars-runtime-32==1.43.2` requirement (also MIT) | -| pyarrow | 25.0.0 | core | Apache-2.0 | permissive | -| scipy | 1.18.0 | core | BSD-3-Clause (project) — from `LICENSE.txt` | metadata carries only the coarse `BSD License` classifier; the shipped `LICENSE.txt` is the three-clause text, including the non-endorsement clause. **The binary wheel bundles non-permissive shared libraries** — see below | -| torch | 2.13.0+cu126 | core | BSD-3-Clause (project); wheel declares `Apache-2.0 AND Apache-2.0 WITH LLVM-exception AND BSD-2-Clause AND BSD-3-Clause AND BSL-1.0 AND MIT` | **new since the 2026-07-25 audit:** 2.13 ships a real `License-Expression` covering its vendored third-party code, plus 97 `licenses/third_party/` files alongside its own `LICENSE` (still the PyTorch BSD-3-Clause text). The compound expression is *torch's own* vendored set — it does **not** cover the `cu126` build's **proprietary** NVIDIA CUDA libraries, which remain outside the wheel; see below | -| pytest | 9.1.1 | dev | MIT | permissive | -| ruff | 0.15.22 | dev | MIT | permissive | -| scanpy | 1.12.3 | dev | BSD-3-Clause | permissive | -| PyYAML | 6.0.3 | dev | MIT | permissive | -| numba | 0.66.0 | fast | BSD-2-Clause — from `LICENSE` | metadata carries only the coarse `BSD License` classifier; the shipped `LICENSE` has two clauses and no non-endorsement clause, hence 2- rather than 3-clause | -| shardad | 0.7.1 | streaming, streaming-gpu | MIT | Arc Institute private package, pinned by `[tool.uv.sources]` to its `v0.7.1` tag. `streaming` takes `shardad[cell]`, `streaming-gpu` takes `shardad[cell,gpu]` (`pyproject.toml:51,56`). **No structured identifier**; taken from the `LICENSE` shipped in the wheel (MIT, © 2026 Arc Research Institute). The `[cell]` extra pulls `pyfastpfor` — see below | - -### SciPy's bundled shared libraries - -The manylinux wheel vendors compiler-runtime and BLAS libraries into -`scipy.libs/`. These are **declared by the wheel**, in the legacy `License` -metadata value, each with an SPDX identifier and the file it covers: - -| Bundled component | Files | License | -|---|---|---| -| OpenBLAS | `libscipy_openblas*.so` | BSD-3-Clause | -| LAPACK | `libscipy_openblas*.so` | BSD-3-Clause-Open-MPI | -| GCC runtime library (`libgfortran`) | `libgfortran*.so` | GPL-3.0-or-later WITH GCC-exception-3.1 | -| libquadmath | `libquadmath*.so` | LGPL-2.1-or-later | - -(The wheel's CycloneDX SBOM, `/sboms/auditwheel.cdx.json`, lists -`libgfortran` and `libquadmath` as components but carries **no** license -entries for them, and does not inventory `libscipy_openblas` at all. The legacy -`License` value is the authoritative source here, not the SBOM.) - -This matters only for **redistribution** of an assembled environment, not for -using gpudge. Two notes on the copyleft entries: - -- **GCC-exception-3.1** permits Target Code that combines the runtime library - with Independent Modules to be conveyed under terms of your choice, provided - the Target Code came from an Eligible Compilation Process. That is what stops - GPL copyleft reaching the qualifying combined work — it does **not** make - `libgfortran` itself permissive; that library remains GPL-covered with its own - redistribution obligations. -- **`libquadmath` is LGPL-2.1-or-later** with no such exception, so it carries - the usual LGPL notice, source-availability and relinking obligations on - redistribution. - -## Transitive dependencies requiring attention - -### The `[cell]` extra: `pyfastpfor` and `pybind11` - -New since the 2026-07-25 audit. Both arrive through the `streaming` extras. Only the -first edge of the chain is visible in gpudge's own `pyproject.toml` (lines 51, 56); -the rest is not, which is why neither package appeared in earlier audits: - -``` -gpudge[streaming] -> shardad[cell]>=0.7.1 (pyproject.toml:51) -gpudge[streaming-gpu] -> shardad[cell,gpu]>=0.7.1 (pyproject.toml:56) -shardad[cell] -> pyfastpfor>=1.4 (shardad's `extra == "cell"`) -pyfastpfor -> pybind11>=2.4 (unconditional Requires-Dist) -``` - -`pyfastpfor` is not optional at install time: shardad requires it with no condition -beyond `extra == "cell"`, and both of gpudge's streaming extras select that extra, -so every `streaming` install pulls it in. At runtime shardad imports it -lazily, and only for `codec='pfordelta'` cell archives — `codec='zstd'` opens -without it — but `pfordelta` is what every production `.csad` uses. - -| Package | Version | License | Notes | -|---|---|---|---| -| pyfastpfor | 1.4.0 | Apache-2.0 **per the release's own source headers** — no license file in any published artifact | see flag 6 | -| pybind11 | 3.0.4 | BSD-3-Clause | permissive; declares a real `License-Expression` **and** ships `LICENSE` (© 2016 Wenzel Jakob). No gap | - -**`pyfastpfor` ships no license file in any artifact.** Being precise about *which* -artifact matters here: - -- **PyPI publishes only an sdist** for 1.4.0 — `pyfastpfor-1.4.0.tar.gz`, uploaded - 2023-10-14, with an empty `license` field and an empty `classifiers` array. There - is **no published wheel**; the `.dist-info` seen in an installed environment - belongs to a wheel the resolver **built locally** from that sdist. -- **The sdist contains no license file.** All 75 entries scanned: `MANIFEST.in`, - `PKG-INFO`, `fastpfor/`, `pyfastpfor.cc`, `pyfastpfor.egg-info`, - `requirements.txt`, `setup.cfg`, `setup.py` — nothing matching - `licen|notice|copying`. -- **The locally built wheel loses even the in-source notices.** Seven files: - - ``` - pyfastpfor-1.4.0.dist-info/{INSTALLER,METADATA,RECORD,REQUESTED,WHEEL,top_level.txt} - pyfastpfor.cpython-312-x86_64-linux-gnu.so (17 MB) - ``` - - No `License-Expression`, no legacy `License`, no `License ::` classifier, no - `License-File`, and no `LICENSE`/`COPYING`/`NOTICE`. `strings` over the `.so` - matches no occurrence of `apache`, `copyright`, or `licen[sc]e` (a pattern - deliberately spanning both spellings) either — the - compiled artifact carries the grant nowhere at all. - -**The grant itself is not in doubt, and the citation is the release's own source** — -not GitHub's current license detector, whose repository-level `LICENSE` is not part -of the 1.4.0 sdist: - -- `pyfastpfor.cc`, header comment: *"This code is released under the Apache License - Version 2.0 http://www.apache.org/licenses/."* -- **51 of the 58** files in the bundled `fastpfor/` tree carry an `Apache License` - notice. Upstream `lemire/FastPFor` states *"This code is licensed under Apache - License, Version 2.0 (ASL2.0)"*. - -So the terms are Apache-2.0 and permissive. What is missing is artifact-level: the -license copy that **Apache-2.0 §4(a)** obliges a redistributor to give recipients. -§4(d)'s attribution-`NOTICE` duty is **conditional on the original work carrying a -`NOTICE` file**, and neither upstream repository has one — `searchivarius/PyFastPFor` -ships `LICENSE` + `README.md`, `lemire/FastPFor` ships `LICENSE` + `AUTHORS`. So -§4(a) alone is unmet. - -### NVIDIA CUDA runtime - -The `cu126` PyTorch build (plus `streaming-gpu`) pulls these in. The set is -**mostly proprietary** NVIDIA software; the open-source exceptions are marked. -Installation as a Torch runtime dependency does not by itself establish that -gpudge exercises every library listed. - -Package names and license strings below are exactly as installed and declared — -the core CUDA set that arrives with Torch uses **four** different spellings for -what is effectively the same proprietary grant, and `nvidia-nvcomp-cu12` (via -`streaming-gpu`) names its SDK agreement separately, for five proprietary strings -in all. They are grouped by the string each one actually carries rather than -lumped together. - -| Package(s) | Declared license | -|---|---| -| `nvidia-cublas-cu12`, `nvidia-cuda-cupti-cu12`, `nvidia-cuda-nvrtc-cu12`, `nvidia-cuda-runtime-cu12`, `nvidia-cudnn-cu12`, `nvidia-cufft-cu12`, `nvidia-cufile-cu12`, `nvidia-curand-cu12`, `nvidia-cusolver-cu12`, `nvidia-cusparse-cu12`, `nvidia-nvjitlink-cu12` | `Other/Proprietary License` (NVIDIA EULA) | -| `nvidia-cusparselt-cu12` | `NVIDIA Proprietary Software` | -| `nvidia-nccl-cu12`, `nvidia-nvshmem-cu12` | `LicenseRef-NVIDIA-Proprietary` | -| `cuda-bindings` | `LicenseRef-NVIDIA-SOFTWARE-LICENSE` | -| `nvidia-nvcomp-cu12` | `LICENSE AGREEMENT FOR NVIDIA SOFTWARE DEVELOPMENT KITS` — via `streaming-gpu` (shardad's GPU decode) | -| `cuda-toolkit` | **none at all** — no identifier and no shipped license file. An empty NVIDIA metapackage; treat as undeclared/unknown rather than assuming particular terms. See flag 4 | -| `cuda-pathfinder` | Apache-2.0 ✅ | -| `nvidia-nvtx-cu12` | Apache-2.0 ✅ — per its `License` value and shipped Apache text; note its classifier contradictorily says `Other/Proprietary License` | -| `triton` | MIT ✅ | -| `cupy-cuda12x` | MIT ✅ — via `streaming-gpu` | - -### MPL-2.0 (weak, file-level copyleft) - -- `certifi` (MPL-2.0) -- `tqdm` (MPL-2.0 AND MIT) -- `fast-array-utils`, `legacy-api-wrap`, `session-info2` (via the `scanpy` dev extra) - -**Mere internal use does not trigger MPL distribution or source-availability -obligations.** On **distribution in executable form**, MPL-2.0 §3.2 requires -that the Source Code Form of the covered files be made available under the MPL, -together with the required notices — this applies whether or not you modified -them. "File-level" copyleft bounds *which files* the obligation reaches (not -unrelated files in the Larger Work); it does not mean the obligation attaches -only to files you changed. - -### Everything else - -The remaining transitive packages in gpudge's dependency closure are permissive -— MIT / BSD / Apache-2.0 / PSF. (No count is given deliberately: it drifts with -every dependency bump.) - -## Compliance flags - -1. **Proprietary NVIDIA CUDA components are present.** Most of the CUDA runtime - set is proprietary (the Apache/MIT exceptions are marked in the table above), - and `streaming-gpu` adds `nvidia-nvcomp-cu12`. This does not change gpudge's - own MIT licensing, but use *and* redistribution of those components remain - governed by NVIDIA's terms — which is what blocks redistributing the - assembled binary environment under open terms. - -2. **Three packages ship a license but declare no identifier.** `shardad` - (MIT), `hdf5plugin` (MIT, wrapper only — see flag 3), and `google-crc32c` - (Apache-2.0, a direct requirement of `zarr`; `numcodecs` offers it only under - an optional extra that is not active here) each have a `License-File` and a - real grant, but no expression, legacy value, or classifier. No *grant* is - missing; this is an SBOM/tooling defect. Scanners keyed on the identifier - fields cannot resolve these from those fields, whereas scanners that read - `License-File` can. Worth adding a license expression to shardad's - `pyproject.toml` before any public / PyPI release that references it. - - *Resolved since the previous audit:* shardad had no `LICENSE` file at all at - 0.2.0, which that audit correctly flagged as all-rights-reserved by default. - It has since been licensed MIT. - - A neighbouring case where the identifier fields *are* populated but say nothing: - `umap-learn` (transitive, via the `scanpy` dev extra) declares the bare - classifier `License :: OSI Approved` — naming no license family — over a legacy - `License` value of just `BSD`. Its shipped `LICENSE.txt` is BSD 3-Clause - (© 2017 Leland McInnes). Same remedy, same non-issue for rights. - -3. **`hdf5plugin`'s bundled filters are not covered by its MIT grant.** The - shipped `LICENSE` grants MIT for the hdf5plugin code itself; the bundled HDF5 - filters have their own terms, and the wheel includes neither their notices - nor upstream's `doc/information.rst` enumerating them. Redistributing the - assembled artifact needs a separate review of those filter licenses — this is - not merely a metadata gap. - -4. **`cuda-toolkit` has no grant to point at.** No identifier, no `License-File`, - no license text in the wheel. It is an empty NVIDIA metapackage, so exposure - is **low — not nil**: its redistribution status is genuinely unknown rather - than resolved, and the installed wheel does not establish which NVIDIA terms - govern. - -5. **SciPy's wheel bundles `LGPL-2.1-or-later` (`libquadmath`) and - `GPL-3.0-or-later WITH GCC-exception-3.1` (`libgfortran`).** Use is - unaffected; redistribution of the assembled environment is not purely - permissive. - -6. **`pyfastpfor` ships no license copy in any published artifact, and it is an - unconditional member of the `streaming` closure.** shardad requires it with no - condition beyond `extra == "cell"` (`Requires-Dist: pyfastpfor>=1.4; extra == - "cell"`), and both streaming extras select that extra, so installing either one - always installs it — which is what puts it in scope here. (At *runtime* it is imported - lazily and only for `codec='pfordelta'` cell archives; shardad's cell layout - also supports `codec='zstd'`, which opens without it. `pfordelta` is what every - production `.csad` uses.) PyPI carries only an sdist for 1.4.0, - and that sdist has no `LICENSE`/`NOTICE`/`COPYING` among its 75 entries; the - wheel a resolver builds from it is seven files with no license metadata and a - `.so` that embeds no license text. The grant is nevertheless clear from the - release's **own source** — `pyfastpfor.cc`'s header states Apache-2.0, as do 51 - of the 58 bundled `fastpfor/` files — so the **terms are permissive and the - license is established**; what is absent is the artifact-level copy. - **Apache-2.0 §4(a)** requires a redistributor to give recipients a copy of the - License, so **redistributing an assembled environment containing this package - means adding that copy yourself, from an authoritative source** — §4(a) governs - conveying the text, not where you obtain it, and the package supplies it - nowhere. (§4(d)'s `NOTICE` duty is conditional on the original work having a - `NOTICE` file; neither `searchivarius/PyFastPFor` nor `lemire/FastPFor` has one, - so it does not apply.) This is a **missing-license-copy** problem: strictly worse - than flag 2's packages, which ship a real grant and merely fail to declare an - identifier, and distinct from flag 3's hdf5plugin case, which is a - missing-*notice* problem for components under terms the wheel never states. - Worth reporting upstream. - -**Net:** everything gpudge itself authors is MIT, and every direct dependency's -*project-level* license is permissive. `pyfastpfor`, although transitive rather than -direct, is likewise Apache-2.0 at the project level even though its published -artifacts carry no license copy (flag 6). **Using** gpudge triggers no copyleft or -disclosure obligation. - -**Redistributing an assembled environment is a separate question and is not -purely permissive.** It carries NVIDIA's proprietary terms; MPL-2.0 §3.2 -source-availability for five packages; SciPy's bundled `LGPL-2.1-or-later` and -`GPL-3.0-or-later WITH GCC-exception-3.1` runtime libraries (so the installed SciPy *artifact* is -not purely permissive even though the SciPy *project* is BSD-3); hdf5plugin's -bundled-filter terms, whose notices the wheel does not ship; `pyfastpfor`'s -Apache-2.0 license copy, which §4(a) obliges a redistributor to convey and which -must be added from an authoritative source because no published artifact contains -it (flag 6); and `cuda-toolkit`, whose governing terms are simply unknown. -Separately, the identifier gaps in -flag 2 — and `umap-learn`'s content-free classifier — affect SBOM generation -rather than rights; flag 6 is **not** in that category, because a missing license -copy is a redistribution obligation, not a tooling defect. diff --git a/docs/release-notes/v0.3.0.md b/docs/release-notes/v0.3.0.md index 7fd30e3..dbff93f 100644 --- a/docs/release-notes/v0.3.0.md +++ b/docs/release-notes/v0.3.0.md @@ -10,7 +10,8 @@ ArcInstitute SSH access); gpudge is not on PyPI. ## 🌊 Shard-streaming `de(shard_archive=…)` -Stream a [shardad](https://github.com/ArcInstitute/shardad) target-aware v2 +Stream a [shardad](https://github.com/ArcInstitute/cellstream) — since renamed +**cellstream**, which is where that link goes — target-aware v2 archive directly, keeping the reference pool resident-sorted on the GPU and streaming guide-shards past it. Host RAM is bounded to ~reference + one shard — the full **5.54 M-cell CCL_2** screen runs in **~31 GB host RAM** on a single diff --git a/environment.yml b/environment.yml index d7a6bf1..9021286 100644 --- a/environment.yml +++ b/environment.yml @@ -6,7 +6,7 @@ # The gpudge pin is EXACT, unlike the README's `pip install gpudge`. The two are # for different things: someone following the README wants the current release, # while this file is checked in beside the code it belongs to, so unpinned, -# `mamba env create` from a v0.9.0 checkout would install whatever PyPI has +# `mamba env create` from a v0.9.1 checkout would install whatever PyPI has # latest. It is one of the files a release cut must bump, and it always has # been; it was pinned to a git tag before gpudge was published and stays pinned # now that it resolves from PyPI. @@ -27,8 +27,8 @@ # For the optional `de(archive=...)` streaming path, switch the extra to # [fast,streaming]; cellstream resolves from PyPI and needs no separate step. # (For GPU device decode use [fast,streaming-gpu], which adds cellstream's -# [gpu] extra. cellstream is the former shardad, renamed and first published -# at 0.9.0, which is the floor for both archive layouts.) +# [gpu] extra. cellstream's floor is 0.9.0, the first release carrying both +# archive layouts.) name: gpudge channels: - conda-forge @@ -37,4 +37,4 @@ dependencies: - pip - pip: - --extra-index-url https://download.pytorch.org/whl/cu126 - - "gpudge[fast]==0.9.0" + - "gpudge[fast]==0.9.1" diff --git a/pyproject.toml b/pyproject.toml index 9336607..1836858 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "gpudge" -version = "0.9.0" +version = "0.9.1" description = "Lightweight GPU-only Mann–Whitney U differential expression." readme = "README.md" authors = [{ name = "Arc Institute" }] @@ -61,9 +61,9 @@ fast = ["numba>=0.59"] # Optional: streaming a cellstream archive directly (de(archive=...)). # The core library takes an in-memory AnnData and does NOT need cellstream. # `streaming` is the CPU-friendly base: pulls no CUDA-only wheels onto host-only -# / CPU-CI installs. cellstream is the former `shardad`, renamed and published -# to PyPI at 0.9.0; the floor is that first public release rather than the old -# private v0.7.1 pin. The [gpu] extra is cupy + nvcomp, needed for device +# / CPU-CI installs. The floor is 0.9.0, cellstream's first PyPI release, which +# is also the first version carrying both archive layouts. +# The [gpu] extra is cupy + nvcomp, needed for device # decode; it belongs # here rather than surfacing as a bare ImportError at open time. # @@ -94,11 +94,10 @@ explicit = true [tool.uv.sources] torch = [{ index = "pytorch-cu126", marker = "platform_system == 'Linux'" }] -# NOTE: there is deliberately no `cellstream` entry here. It used to be a -# private git-SSH source pinned to `shardad`'s v0.7.1 tag; since the rename it -# resolves from PyPI like any other dependency, so the `streaming` extra above -# is the whole story. Removing it also removes the reason CI had to use -# `uv pip install` rather than `uv sync` -- see .github/workflows/ci.yml. +# NOTE: there is deliberately no `cellstream` entry here. It resolves from PyPI +# like any other dependency, so the `streaming` extra above is the whole story -- +# which is also why CI no longer has to prefer `uv pip install` over `uv sync`; +# see .github/workflows/ci.yml. # # Two caveats that hung off the old pin are gone with it: cellstream's Rust # extension is no longer optional (a source build without a toolchain now fails diff --git a/src/gpudge/__init__.py b/src/gpudge/__init__.py index 99d0330..d3b8847 100644 --- a/src/gpudge/__init__.py +++ b/src/gpudge/__init__.py @@ -58,7 +58,7 @@ warnings.warn( "gpudge: numba is not installed; falling back to scipy " "for sparse CSR row slicing (~3x slower on multi-million-cell " - "inputs). Install with `pip install gpudge[fast]` (or " + "inputs). Install with `pip install 'gpudge[fast]'` (or " "`uv pip install -e \".[fast]\"`) to enable the numba kernel.", stacklevel=2, ) @@ -1272,11 +1272,13 @@ def _finish(result): labels_t = torch.from_numpy(state.labels).to(device) # Auto-pick gene chunk size from free GPU memory if not provided. - # Heuristic adopted from pdex's default_gene_chunk_size: working memory is - # dominated by ref-cell ranking buffers (~24 bytes per ref cell per gene: - # float32 values + int64 sort indices + float64 ranks + workspace), capped - # at 16 GB and 20% of free GPU memory. For all_others, ranks cover every - # cell (1-vs-rest semantics), so use n_cells as the budget basis instead. + # Heuristic adapted from `default_gene_chunk_size` in the `gpu` branch of + # abhinadduri/pdex -- the GPU fork, not upstream pdex; see AUTHORS. Working + # memory is dominated by ref-cell ranking buffers (~24 bytes per ref cell per + # gene: float32 values + int64 sort indices + float64 ranks + workspace), + # capped at 16 GB and 20% of free GPU memory. For all_others, ranks cover + # every cell (1-vs-rest semantics), so use n_cells as the budget basis + # instead. # # In ref-mode we also allocate per-chunk GPU accumulators of shape # (n_groups, ch) × float64. Count: arithmetic / U / p (= 3) for diff --git a/src/gpudge/_cell_stream.py b/src/gpudge/_cell_stream.py index dcd5a34..6951217 100644 --- a/src/gpudge/_cell_stream.py +++ b/src/gpudge/_cell_stream.py @@ -7,7 +7,7 @@ from ._ingest import reject_missing_group_labels from ._csr_dense import csr_row_sums -from ._stream_backend import validate_archive_reference +from ._stream_backend import label_str, validate_archive_reference logger = logging.getLogger(__name__) @@ -67,42 +67,107 @@ def _plan_batches(ranges, *, bytes_per_row, target_bytes=None): def _cell_group_ranges(store): """``(reference_labels | None, [(label, start, stop), ...])`` for a CellStore. - Reads cellstream's **private** ``CellStore._load_groups()``. That upstream - ask has since been answered: cellstream 0.8.0 added public - ``CellStore.group_spans()`` (a list of ``GroupSpan(label, start, stop)``) and - ``CellStore.reference_row_count()``, which give exactly this table without - loading obs. Migrating to them is tracked separately and is a cleanup, not a - fix -- the private call still exists in the pinned 0.9.0 and the suites pass - against it. Until then the reasoning below is history, not a live claim: run-detection - over ``store.obs[group_by]`` would load the whole obs DataFrame -- - 36 columns x 1.27 M rows on a production archive -- to recover a table the - archive already stores, which is why the private reach-in was taken; ``test_cell_group_ranges_private_api_gone`` makes - its removal fail loudly rather than silently. - - Validates what cellstream's own ``read_reference()`` silently assumes: the - reference groups lead contiguously from row 0, so its ``[0, max stop)`` - gather cannot pull target cells into the reference pool. + Reads cellstream's **public** ``CellStore.group_spans()`` -- every group's + ``[start, stop)`` row range in storage order -- plus the manifest's + ``reference``. gpudge used to reach into the private ``_load_groups()`` + because no public equivalent existed; cellstream 0.8.0 added one (upstream + #248) and the ``>=0.9.0`` floor makes it always available. + ``test_cell_group_ranges_public_api_gone`` makes its removal fail loudly + rather than silently. Neither call reads obs: run-detection over + ``store.obs[group_by]`` would load the whole DataFrame -- 36 columns x 1.27 M + rows on a production archive -- to recover a table the archive already stores. + + ``reference_row_count()`` is deliberately NOT used as the source of the + reference block's end. The label split below has to happen regardless, and + deriving both from one table makes them consistent by construction instead + of by agreement. + + **Duplicate labels are rejected, and upstream declares them LEGAL.** Under + the default sort order, distinct raw obs values that stringify identically + produce several records carrying one label -- which is exactly why + ``group_spans()`` returns a list and not a mapping. For a **target** label + gpudge genuinely cannot handle it, and the mechanism is worth stating + precisely because it is not "the spans collapse into one group": + ``_CellBackend._targets`` keeps BOTH entries, while + ``_tgt_index = {lab: i for i, lab in enumerate(...)}`` maps both to the LAST + of them. So one accumulator row is never written at all and the other is + overwritten span by span, leaving the final span's subset -- a wrong answer in + two rows rather than a short read in one. + + For a **reference** label the rejection is stricter than it needs to be: + duplicate reference spans are handled correctly downstream + (``_ref_stop`` takes the max stop, and ``_target_ranges`` excludes every + reference-labelled span), so gpudge could accept them. It does not, because + that path has no fixture and no coverage, and loosening it is not what this + change is for. Tracked as a follow-up rather than done here. + + The tiling, coverage and reference-prefix checks below are kept, but they are + **future-contract assertions, not live cross-checks**: ``group_spans()`` + routes through ``_load_groups()``, which runs cellstream's own + ``_validate_groups_record`` first, so against any real cellstream >=0.9.0 + store a violation raises ``IncompatibleSchemaError`` upstream and these can + never fire. They exist so that a cellstream which stops validating fails here + instead of silently mis-slicing, and the fake-store tests exercise that + fallback -- not a production path. What IS load-bearing today is the call + ORDER: validation-before-use, not merely reading the manifest second. """ - # The per-record parse is INSIDE the try: a record missing 'label'/'start'/ - # 'stop', or carrying a non-numeric bound, is the same class of failure as - # the table itself changing shape, and should get the same explanatory - # error rather than a bare KeyError/ValueError from a list comprehension. + # One `where` for every message below, computed with getattr and therefore + # incapable of raising: a store whose contract has moved far enough may not + # carry `path`, and `store.path` inside the except block would replace the + # explanation with "during handling of the above exception, another exception + # occurred" -- while the same access in the ValueError paths would raise a + # bare AttributeError instead of the error those paths exist to give. Doing + # it once is also how the two stay consistent; being defensive in the except + # block alone was a half-measure. (Gemini review, PR #146.) + where = getattr(store, "path", "") + + # The per-span parse is INSIDE the try: a span that does not unpack into + # three values, or carries a non-numeric bound, is the same class of failure + # as the method disappearing, and should get the same explanatory error + # rather than a bare TypeError/ValueError from a list comprehension. try: - rec = store._load_groups() - raw = rec["groups"] - ref = rec["reference"] - ranges = [(str(g["label"]), int(g["start"]), int(g["stop"])) for g in raw] - except (AttributeError, KeyError, TypeError, ValueError) as e: + # label_str, not str(): a bytes label would become "b'ntc'" and reach the + # output frame as a label nobody wrote. Upstream types labels as `str`, so + # this is contract insurance rather than a live case -- but both sides of + # the comparison below have to normalize the same way, and they now share + # one helper. (Gemini review, PR #146.) + ranges = [(label_str(lab), int(start), int(stop)) + for lab, start, stop in store.group_spans()] + # AFTER group_spans(): loading the spans is what type-checks the + # manifest's 'reference' and cross-checks it against groups.json's own + # copy, so reading it first would read an unvalidated value. + # + # `.get`, not `["reference"]`, and that is deliberate -- a reviewer asked + # for the subscript so a missing key would fail loudly. Measured instead: + # a cell archive with NO reference and no `reference` key is VALID + # upstream (group_spans() returns its spans), so the subscript would + # refuse a good archive; and an archive that DOES have a reference but + # whose manifest lost the key already raises IncompatibleSchemaError from + # _validate_groups_record's manifest-vs-groups.json check, above, so the + # silent-misread this would guard against cannot reach here. + # (Copilot review, PR #146.) + ref = store.manifest.get("reference") + except (AttributeError, KeyError, TypeError, ValueError, OverflowError) as e: + # OverflowError is in the tuple because `int(float("inf"))` raises it + # rather than ValueError, so an inf-bounded span would otherwise escape + # this wrapper. NaN is NOT such a case -- `int(float("nan"))` raises + # ValueError, which was already caught. (codex; corrected by Copilot.) + # The message names BOTH surfaces because the try covers both: a + # `manifest` that is None or lacks `.get` lands here too, and blaming + # group_spans() for it would point at the call that succeeded. + # (Gemini review, PR #146.) raise RuntimeError( - f"gpudge cannot read the group table of {store.path}: cellstream's " - f"private CellStore._load_groups() did not return the expected " - f"{{'reference': ..., 'groups': [...]}} shape. gpudge pins " - f"cellstream>=0.9.0; see gpudge _cell_stream._cell_group_ranges." + f"gpudge cannot read the group table of " + f"{where}: cellstream's " + f"CellStore.group_spans() did not return the expected list of " + f"(label, start, stop) spans, or CellStore.manifest is not a mapping. " + f"gpudge pins cellstream>=0.9.0; see " + f"gpudge _cell_stream._cell_group_ranges." ) from e if not ranges: raise ValueError( - f"{store.path}: cell archive has no group table (was it written " + f"{where}: cell archive has no group table (was it written " f"with group_by=?)." ) labels = [lab for lab, _, _ in ranges] @@ -111,7 +176,7 @@ def _cell_group_ranges(store): for lab in labels: (dup if lab in seen else seen).add(lab) raise ValueError( - f"{store.path}: duplicate group labels {sorted(dup)} in the group table." + f"{where}: duplicate group labels {sorted(dup)} in the group table." ) # An unassigned cell reaches the archive as a group literally named 'nan' # (cellstream stringifies the obs column at write time), which the in-memory @@ -121,37 +186,37 @@ def _cell_group_ranges(store): # this function exists to avoid, so the string level is the right level. # (ultrareview 2026-08) reject_missing_group_labels( - labels, where=str(store.path), + labels, where=str(where), remedy="Drop or assign the unassigned cells before writing the archive.") pos = 0 for lab, start, stop in ranges: if start != pos or stop < start: raise ValueError( - f"{store.path}: group ranges do not tile [0, n_obs) -- group " + f"{where}: group ranges do not tile [0, n_obs) -- group " f"{lab!r} is [{start}, {stop}) but the previous group ended at " f"{pos}." ) pos = stop if pos != int(store.n_obs): raise ValueError( - f"{store.path}: group ranges cover {pos} rows but the archive has " + f"{where}: group ranges cover {pos} rows but the archive has " f"{int(store.n_obs)}." ) if ref is None: return None, ranges - ref_labels = [str(x) for x in (ref if isinstance(ref, list) else [ref])] + ref_labels = [label_str(x) for x in (ref if isinstance(ref, list) else [ref])] label_set = set(labels) # hoisted: rebuilt per reference label otherwise missing = [lab for lab in ref_labels if lab not in label_set] if missing: raise ValueError( - f"{store.path}: the manifest names reference label(s) {missing} that " + f"{where}: the manifest names reference label(s) {missing} that " f"are not in the group table." ) lead = {lab for lab, _, _ in ranges[:len(ref_labels)]} if lead != set(ref_labels): raise ValueError( - f"{store.path}: reference labels {sorted(ref_labels)} are not the " + f"{where}: reference labels {sorted(ref_labels)} are not the " f"leading groups (rows from 0 start with {sorted(lead)}). cellstream's " f"read_reference() gathers [0, max reference stop), so a non-leading " f"reference block would silently include target cells in the " diff --git a/src/gpudge/_csr_dense.py b/src/gpudge/_csr_dense.py index b0adca4..6f36b28 100644 --- a/src/gpudge/_csr_dense.py +++ b/src/gpudge/_csr_dense.py @@ -7,7 +7,7 @@ remaining post-FDR-fix bottleneck). One pass + parallel-rows kernel removes most of that overhead. -Install with ``pip install gpudge[fast]`` (or ``uv pip install -e +Install with ``pip install 'gpudge[fast]'`` (or ``uv pip install -e ".[fast]"``) to enable; the extra is just ``numba``. """ from __future__ import annotations diff --git a/src/gpudge/_stream_backend.py b/src/gpudge/_stream_backend.py index ad6a103..feb6ac8 100644 --- a/src/gpudge/_stream_backend.py +++ b/src/gpudge/_stream_backend.py @@ -50,6 +50,30 @@ from __future__ import annotations +def label_str(x): + """One group/reference label as a `str`, decoding bytes rather than repr-ing it. + + `str(b"ntc")` is `"b'ntc'"` -- the Python 3 str(bytes) trap -- which would + reach the output frame as a label nobody wrote. `str(...)` on the str arm too: + `np.str_` subclasses `str` and would otherwise leak out as `np.str_`. + + Module level, and used by BOTH `validate_archive_reference` and + `_cell_stream._cell_group_ranges`, so the two cannot disagree about what a + label is. It was a nested helper in the former until a review pointed out the + latter was missing it. (Gemini review, PR #146.) + + `isinstance`, not `hasattr(x, "decode")`: a reviewer asked for duck-typing on + the grounds that `np.bytes_` might not inherit from `bytes`. Measured -- it + does. `np.bytes_.__mro__` is `(bytes_, bytes, character, flexible, generic, + object)`, `np.dtype("S3").type` IS `np.bytes_`, an element taken from an + S-dtype array satisfies `isinstance(x, bytes)`, and this function already + returns `'ntc'` for all of them. The concern was scoped to NumPy 1.x, which + `pyproject.toml` excludes anyway (`numpy>=2`). Left as isinstance, which is + also the more precise test. (Gemini review, PR #146.) + """ + return x.decode() if isinstance(x, (bytes, bytearray)) else str(x) + + def validate_archive_reference(reference, labels): """Validate ``reference=`` against an archive's OWN reference labels. @@ -70,9 +94,7 @@ def validate_archive_reference(reference, labels): ``reference=['ntc', 'safe'] is not among the archive's reference labels ['ntc', 'safe']``. A sequence is now checked element-wise. """ - def _label(x): - return x.decode() if isinstance(x, (bytes, bytearray)) else str(x) - + _label = label_str # module-level now; see label_str's docstring label_list = sorted(labels) if reference is None: return "|".join(label_list) @@ -114,13 +136,14 @@ def _label(x): def _import_cellstream(): try: import cellstream - except ImportError as e: # pragma: no cover - exercised via monkeypatch + except ImportError as e: + # No `pragma: no cover` here: test_api.py's + # test_missing_cellstream_import_message reaches this branch on CPU CI. raise ImportError( "de(archive=...) requires the optional 'streaming' extra, which " - "installs cellstream (>=0.9.0, the former shardad) from PyPI. " - "Reinstall gpudge with the extra -- e.g. `pip install '.[streaming]'` " - "from a checkout, or add `streaming` to the pin in the README's " - "Install section." + "installs cellstream (>=0.9.0) from PyPI. " + "Reinstall gpudge with the extra -- `pip install 'gpudge[streaming]'`, " + "or `pip install '.[streaming]'` from a checkout." ) from e return cellstream diff --git a/tests/test_api.py b/tests/test_api.py index bcfed98..cdc27de 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -434,6 +434,37 @@ def test_iter_kwargs_serial_and_prefetch(): assert ss._iter_kwargs(8, 4) == {"prefetch": 4, "n_workers": 8} +def test_missing_cellstream_import_message(monkeypatch): + """The missing-streaming-extra error must name BOTH installs that work. + + Lives here, not in test_shard_stream.py, whose module-level + importorskip('cellstream') skips it precisely when cellstream is absent -- + the one situation this message exists for. The test could therefore never + run in the environment it describes. + + Asserts the commands in full. The previous `match="streaming"` is satisfied + by the extra's name alone, so it stayed green while every install command in + the message was rewritten. (codex, checkpoint 2.) + + `sys.modules[name] = None` is the import machinery's own "this module is + unavailable" marker, so it raises ImportError for this one name and leaves + every other import alone -- unlike replacing builtins.__import__, which was + the previous technique and hooks every import in the interpreter for the + duration. Verified against a venv that HAS cellstream installed; without one + the guard is unfalsifiable, because the bare import already fails. + (Gemini review, PR #144.) + """ + import sys + from gpudge._stream_backend import _import_cellstream + + monkeypatch.setitem(sys.modules, "cellstream", None) + with pytest.raises(ImportError) as excinfo: + _import_cellstream() + msg = str(excinfo.value) + assert "pip install 'gpudge[streaming]'" in msg # from PyPI + assert "pip install '.[streaming]'" in msg # from a checkout + + def test_should_device_decode_matrix(monkeypatch): """_should_device_decode truth table (pure helper, no cellstream/cupy needed — monkeypatched). Lives here, not in test_shard_stream.py, whose module-level diff --git a/tests/test_cell_stream.py b/tests/test_cell_stream.py index d65419a..fb5dcba 100644 --- a/tests/test_cell_stream.py +++ b/tests/test_cell_stream.py @@ -46,9 +46,11 @@ def cell_mode2(tmp_path): def test_cell_group_ranges_shape(cell_mode1): - """The private cellstream group table has the shape gpudge depends on, and the + """cellstream's public group_spans() has the shape gpudge depends on, and the ranges tile [0, n_obs) with the reference leading. This test is the tripwire - for cellstream changing CellStore._load_groups().""" + for cellstream changing CellStore.group_spans(), and -- because it reads a + real archive -- for the manifest's `reference` drifting from the labels the + spans carry.""" from gpudge._cell_stream import _cell_group_ranges p, adata = cell_mode1 store = cellstream.open(p) @@ -83,35 +85,42 @@ def test_cell_group_ranges_no_reference(cell_mode2): class _FakeStore: - """Minimal CellStore stand-in for the guard tests.""" + """Minimal CellStore stand-in for the guard tests. + + ``groups`` is a list of ``(label, start, stop)`` triples, handed back as real + ``cellstream.GroupSpan`` values so the fake cannot drift from the type the + store actually returns -- a plain tuple would keep passing after GroupSpan + grew a field or stopped unpacking. + """ path = "" def __init__(self, groups, reference, n_obs): - self._groups = {"reference": reference, "groups": groups} + self._spans = [cellstream.GroupSpan(*g) for g in groups] + self.manifest = {"reference": reference} self.n_obs = n_obs - def _load_groups(self): - return self._groups + def group_spans(self): + return list(self._spans) @pytest.mark.parametrize("groups, reference, n_obs, match", [ # gap between two groups - ([{"label": "a", "start": 0, "stop": 5}, - {"label": "b", "start": 7, "stop": 10}], None, 10, "tile"), + ([("a", 0, 5), ("b", 7, 10)], None, 10, "tile"), # overlap - ([{"label": "a", "start": 0, "stop": 5}, - {"label": "b", "start": 3, "stop": 10}], None, 10, "tile"), + ([("a", 0, 5), ("b", 3, 10)], None, 10, "tile"), # ranges stop short of n_obs - ([{"label": "a", "start": 0, "stop": 5}], None, 10, "cover"), - # duplicate label - ([{"label": "a", "start": 0, "stop": 5}, - {"label": "a", "start": 5, "stop": 10}], None, 10, "duplicate"), + ([("a", 0, 5)], None, 10, "cover"), + # duplicate label -- cellstream ALLOWS these (several records can share a + # stringified label); gpudge cannot. Not because the spans collapse: + # _targets keeps BOTH, while _tgt_index resolves both to the LAST, so one + # accumulator row is never written and the other is overwritten span by + # span. See _cell_group_ranges. (Copilot review, PR #146.) + ([("a", 0, 5), ("a", 5, 10)], None, 10, "duplicate"), # reference label absent from the group table - ([{"label": "a", "start": 0, "stop": 10}], "zzz", 10, "not in the group table"), + ([("a", 0, 10)], "zzz", 10, "not in the group table"), # reference group is NOT leading -- cellstream's read_reference() would gather # [0, ref_stop) and silently pull group 'a' into the reference pool - ([{"label": "a", "start": 0, "stop": 5}, - {"label": "ntc", "start": 5, "stop": 10}], "ntc", 10, "leading"), + ([("a", 0, 5), ("ntc", 5, 10)], "ntc", 10, "leading"), # empty group table ([], None, 0, "no group table"), ]) @@ -121,30 +130,115 @@ def test_cell_group_ranges_guards(groups, reference, n_obs, match): _cell_group_ranges(_FakeStore(groups, reference, n_obs)) -def test_cell_group_ranges_private_api_gone(): - """A cellstream that drops _load_groups() must fail loudly, not silently.""" +@pytest.mark.parametrize("manifest", [None, object(), 42], + ids=["none", "no-get", "not-a-mapping"]) +def test_cell_group_ranges_bad_manifest_is_wrapped(manifest): + """A `manifest` that is not a mapping lands in the SAME wrapper as a broken + group_spans(), because one try covers both -- so the message has to name both + surfaces. Before it did, this case reported that group_spans() had not + returned the expected spans, blaming the call that succeeded. + (Gemini review, PR #146.)""" + from gpudge._cell_stream import _cell_group_ranges + + class _BadManifest: + path = "" + n_obs = 10 + + def group_spans(self): + return [cellstream.GroupSpan("a", 0, 10)] + + store = _BadManifest() + store.manifest = manifest + with pytest.raises(RuntimeError, match="manifest"): + _cell_group_ranges(store) + + +def test_cell_group_ranges_without_path_still_explains(): + """A store lacking `path` must still get the explanatory error. + + Every message in _cell_group_ranges names the archive, and each one used to + read `store.path` directly. On a store whose contract has moved far enough to + drop it that raised a bare AttributeError instead -- in the except block it + would additionally have masked the original exception. They all route through + one getattr-computed local now, so the guard still speaks. + (Gemini review, PR #146.)""" + from gpudge._cell_stream import _cell_group_ranges + + class _NoPath: # deliberately no `path` + n_obs = 10 + manifest = {"reference": None} + + def group_spans(self): # a gap: does not tile [0, n_obs) + return [cellstream.GroupSpan("a", 0, 4), + cellstream.GroupSpan("b", 6, 10)] + + with pytest.raises(ValueError, match=".*tile"): + _cell_group_ranges(_NoPath()) + + +def test_cell_group_ranges_decodes_bytes_labels(): + """A bytes label must decode, not stringify to "b'ntc'". + + `str(b"ntc")` is the Python 3 str(bytes) trap: the label would reach the + output frame as a name nobody wrote, and the reference-membership comparison + would be made between two reprs. Upstream types labels as `str`, so this is + contract insurance -- but BOTH sides of that comparison (the spans and the + manifest's reference) now normalize through the same `label_str` helper, and + this pins it. (Gemini review, PR #146.)""" + from gpudge._cell_stream import _cell_group_ranges + + ref_labels, ranges = _cell_group_ranges( + _FakeStore([(b"ntc", 0, 4), (b"A", 4, 10)], b"ntc", 10)) + assert ref_labels == ["ntc"] + assert ranges == [("ntc", 0, 4), ("A", 4, 10)] + + +def test_cell_group_ranges_public_api_gone(): + """A cellstream that drops group_spans() must fail loudly, not silently.""" from gpudge._cell_stream import _cell_group_ranges class _NoGroups: path = "" n_obs = 10 + manifest = {"reference": None} - with pytest.raises(RuntimeError, match="_load_groups"): + with pytest.raises(RuntimeError, match="group_spans"): _cell_group_ranges(_NoGroups()) @pytest.mark.parametrize("bad", [ - {"start": 0, "stop": 5}, # missing 'label' - {"label": "a", "stop": 5}, # missing 'start' - {"label": "a", "start": 0, "stop": "five"}, # non-numeric bound -], ids=["no-label", "no-start", "bad-bound"]) -def test_cell_group_ranges_malformed_record_is_wrapped(bad): - """A malformed group RECORD is the same class of failure as the table - changing shape, and must get the same explanatory RuntimeError rather than - a bare KeyError/ValueError escaping a list comprehension.""" + ("a", 0), # too few values to unpack -> ValueError + ("a", 0, 5, 7), # too many -> ValueError + ("a", 0, "five"), # non-numeric bound -> ValueError from int() + ("a", 0, float("inf")), # non-finite bound -> OverflowError from int() + None, # not iterable at all -> TypeError +], ids=["short", "long", "bad-bound", "inf-bound", "not-a-span"]) +def test_cell_group_ranges_malformed_span_is_wrapped(bad): + """A malformed SPAN is the same class of failure as group_spans() itself + changing shape, and must get the same explanatory RuntimeError rather than a + bare TypeError/ValueError escaping a list comprehension. + + Uses a raw list rather than _FakeStore. GroupSpan is a NamedTuple, so its + annotations reject nothing -- `GroupSpan("a", 0, "five")` constructs fine -- + but the arity cases cannot be built through it at all, so the fake would + raise in its own constructor rather than in the code under test. The guard is + for a cellstream whose contract has moved, not for one that is behaving. + + `inf-bound` is the case that motivated widening the except clause: `int()` + raises OverflowError, not ValueError, on a non-finite float, so before that + widening this input escaped the wrapper entirely. (codex.)""" from gpudge._cell_stream import _cell_group_ranges - with pytest.raises(RuntimeError, match="_load_groups"): - _cell_group_ranges(_FakeStore([bad], None, 5)) + + class _BadSpans: + path = "" + n_obs = 5 + manifest = {"reference": None} + + def group_spans(self): + return [bad] + + with pytest.raises(RuntimeError, match="group_spans"): + _cell_group_ranges(_BadSpans()) def test_plan_batches_groups_are_never_split(): @@ -509,9 +603,9 @@ def _write_shard(adata, path, *, group_by, reference): def twin_mode1(tmp_path): """The SAME AnnData written both ways, with DEFAULT ordering. - Default ordering was load-bearing under the old shardad v0.7.1 pin: its cell + Default ordering was load-bearing under the pre-0.8.0 writer: its cell writer converted categorical sort keys with .to_numpy() (losing category - order) where the shard writer used .values (shardad #252), so a categorical + order) where the shard writer used .values (cellstream #252), so a categorical sort_within_group ordered rows differently between layouts and moved the float64 group means. Fixed upstream in 0.8.0, below the >=0.9.0 floor. The fixture keeps default ordering anyway: it is what the twin-mode exactness diff --git a/tests/test_shard_stream.py b/tests/test_shard_stream.py index 5924050..744dd6d 100644 --- a/tests/test_shard_stream.py +++ b/tests/test_shard_stream.py @@ -731,18 +731,6 @@ def test_stream_empty_targets_respects_output_columns(tmp_path): assert df.schema["p"] == pl.Float64 -def test_missing_cellstream_import_message(monkeypatch): - import builtins - from gpudge._stream_backend import _import_cellstream - real_import = builtins.__import__ - def fake(name, *a, **k): - if name == "cellstream": - raise ImportError("no cellstream") - return real_import(name, *a, **k) - monkeypatch.setattr(builtins, "__import__", fake) - with pytest.raises(ImportError, match="streaming"): - _import_cellstream() - def test_resolve_mode1_reference_read_none_raises(archive_mode1, monkeypatch): # Manifest declares a reference shard but read_reference() returns None @@ -1019,7 +1007,7 @@ def test_stream_empty_targets_with_lfc_grid(archive_mode1, tmp_path): # --- tau_star over the streaming paths ----------------------------------- # CI is CPU-only and does not even COLLECT this module (it needs cellstream), so -# these run only on a GPU host with shardad installed. That is precisely why they +# these run only on a GPU host with cellstream installed. That is precisely why they # exist: the tau* accumulators are threaded differently on each path # (`__init__.py` carries a reference row and drops it via target_indices; # `_refpool.py` does not have one at all), and a shape or indexing slip there