fix(deps): make h5py a core dependency; BrainCollection.fit() requires it - #469
Merged
Conversation
…s it
`nltools/data/collection/execution.py` imports h5py unguarded — every
`BrainCollection.fit()` writes an HDF5 fit bundle — but h5py was declared only
in the optional `h5` extra. Anyone installing plain `nltools` hit
ModuleNotFoundError: No module named 'h5py'
raised from inside a joblib worker, which surfaces as a
BrainCollectionWorkerError with a stack that points at joblib rather than at
the missing dependency.
This is invisible in development because the dev group installs h5py, and
invisible to the `h5` extra's own guard: `nltools/io/h5.py` raises a friendly
error for the BrainData `.h5` path, but the collection path never goes through
it.
hdf5plugin stays optional — it only registers blosc/zstd/lz4 filters for
reading compressed legacy files, the bundle writer uses no compression
filters, and `nltools/io/h5.py` already guards that import.
Adds a packaging test that parses pyproject.toml and asserts every module
imported unconditionally by the collection execution path is declared in
[project].dependencies, so the class of bug can't recur silently.
Also refreshes the migration guide, which still described BrainCollection as
"not yet available (scaffold)" and pointed at the deleted
nltools/data/collection/SPEC.md. Replaced with a worked example and the two
contracts that aren't obvious from the signature: the `X=` callable receives a
`_DesignContext` rather than a DesignMatrix, and `from_paths(design_paths=)`
passes paths through unparsed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP
ejolly
force-pushed
the
fix/braincollection-h5py-dependency
branch
from
July 27, 2026 20:29
82f7fff to
0578932
Compare
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.
Problem
BrainCollection.fit()always writes an HDF5 fit bundle, andnltools/data/collection/execution.pyimportsh5pyunguarded:But
h5pywas declared only in the optionalh5extra. Installing plainnltoolsand callingBrainCollection.fit()fails with:Because it is raised inside a joblib worker, the traceback is ~20 frames of
joblib/parallel.pybefore the real cause, which makes it hard to diagnose —especially for students, which is how I found it (porting the
dartbrains course to 0.6).
Two things hid this:
devdependency group installsh5py, so it never reproduces in development.nltools/io/h5.pydoes guard its import and raise an actionable error, butonly for the
BrainData.h5path. The collection path never goes through it.Fix
Move
h5pyto[project].dependencies.BrainCollectionis a headline v0.6feature and is unusable without it, so it is not an optional add-on.
hdf5plugindeliberately stays in theh5extra: it only registersblosc/zstd/lz4 filters for reading compressed legacy files, the bundle writer
uses no compression filters, and
nltools/io/h5.pyalready guards it. Theh5extra keeps listing both so existingnltools[h5]pins resolve unchanged.Regression guard
Adds
nltools/tests/support/test_packaging.py, which parsespyproject.tomland asserts that modules imported unconditionally by the collection execution
path are declared as core dependencies. Written red-first — it fails on
masterwith:The check walks the AST and ignores imports inside
try/except, so guardingan import (with a friendly message) is still a valid way to satisfy it.
Docs
The migration guide still described
BrainCollectionas "not yet available(scaffold)" in three places and pointed at
nltools/data/collection/SPEC.md,deleted in 378c0c1. The class is fully functional — I verified
from_paths→.smooth().fit(model='glm', X=...)→.compute_contrasts()→.ttest()/.permutation_test()/.predict(spatial_scale=...)end to end.Replaced with a worked example plus the two contracts that aren't obvious from
the signatures:
X=callable receives a_DesignContext, not aDesignMatrixfrom_paths(design_paths=...)passes paths through unparsed, so the buildermust construct the
DesignMatrix(matching the source comment: "DesignMatrixhas no read() classmethod yet")
Testing
uv run poe lint— cleannltools/tests/support/— 177 passednltools/tests/data/collection— 276 passed(Both suites need
HF_HUB_DISABLE_IMPLICIT_TOKEN=1on my machine; a stale localHF token 401s against the public
nltools/niftisdataset. Unrelated to this change.)🤖 Generated with Claude Code
https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP