perf: extract file metadata concurrently (thread pool)

[elementalcollision]

Draft PR opened by Chimera (an autonomous code agent operated by @elementalcollision), as part of a review of croissant-baker. Opened as a draft for maintainer review.

What

Per-file metadata extraction — handler selection, whole-file SHA-256, and header/schema reads — is I/O-bound and independent across files, but ran strictly serially in generate_metadata. On datasets with many files (PhysioNet/MIMIC-scale: thousands) that loop is the dominant cost of a bake.

This runs extraction on a ThreadPoolExecutor. The work is off-GIL (file reads + hashlib + pyarrow/pydicom/nibabel header parsing), so threads give real overlap.

Correctness is preserved exactly

The risk with parallelism here is output drift (FileObject @ids are positional). Guarded by construction:

• Worker results are collected by submission index and reassembled in discovery order before any @id is assigned.
• Per-file warnings and the DICOM skip-note are emitted during that in-order pass, so even diagnostic order is unchanged.
• No mlcroissant objects are built off the main thread — workers return plain dicts.

tests/test_parallel.py pins this: byte-identical output across worker counts 1/2/4/8. And the full e2e suite now runs through the auto-parallel path (max_workers=None) with zero change to the committed tests/data/output/*.jsonld — the strongest possible evidence the output is unchanged.

API

• MetadataGenerator(max_workers=None) — auto-sizes to min(8, cpu*2); 1 forces serial.
• New CLI flag --jobs / -j (0 = auto, 1 = serial).
• The progress callback now reports a completion count (one event per file as it finishes).

Benchmark

Synthetic dataset, 60 CSVs / 1.2 GB, 18-core box, warm cache (isolates compute concurrency):

workers	best	speedup
1	1.19s	1.0×
2	0.86s	1.4×
4	0.72s	1.65×
8	0.80s	1.5×

Caveat: This is a warm-cache, CPU-bound measurement (SHA-256 throughput becomes memory-bandwidth bound past ~4 threads, hence the tail-off). On cold or networked storage where PhysioNet-scale datasets actually live, the gain should be larger because per-file I/O latency overlaps across threads. The --jobs flag lets users tune for their storage.

Tests

Full suite: 274 passed (269 + 5 new). No changes to committed outputs.

[rafiattrach]

This is solid: verified byte-identical output serial vs -j8 on the OMOP demo, and
handlers are stateless today so the shared-singleton concurrency is safe. Two asks
before merge, both about keeping it safe as handlers get added:

1. Please add the thread-safety contract to FileTypeHandler.extract_metadata's
   docstring in src/croissant_baker/handlers/base_handler.py (it's a public
   symbol, so this also publishes to the API docs). Suggested, right under the
   summary line "Extract comprehensive metadata from a single file.":

   Thread-safety: extract_metadata may be called concurrently across files on
   a single shared handler instance (handlers are registered as singletons and
   extraction is parallelised — see MetadataGenerator). Implementations must be
   safe to call concurrently: do not mutate shared or instance state during
   extraction. Read-only state set once at construction is fine; mutable
   per-call state on self is a data race.
   

2. A tripwire test so this can't regress silently: see inline comment on
   test_parallel.py.

[rafiattrach]

Could you add a tripwire here that fails CI if a future handler introduces instance
state? The whole parallel path rests on handlers being stateless, and a byte-identical
test won't reliably catch a race. Something like:

def test_registered_handlers_are_stateless() -> None:
    """Extraction runs concurrently on shared handler singletons (#112), so
    handlers must not carry mutable instance state. Fails loudly if one does,
    forcing a conscious thread-safety review before it can ship."""
    from croissant_baker.handlers.registry import (
        get_registered_handlers,
        register_all_handlers,
    )
    register_all_handlers()
    stateful = {type(h).__name__: vars(h) for h in get_registered_handlers() if vars(h)}
    assert not stateful, (
        f"Handlers with instance state: {stateful}. extract_metadata is called "
        "concurrently on a shared instance. If this state is read-only and set at "
        "construction, confirm it's thread-safe and update this test."
    )

[elementalcollision]

Thanks @rafiattrach — both addressed in dcdf179.

1. Docstring contract — added to FileTypeHandler.extract_metadata, close to your wording, extended to also name class/module-level state and parser reentrancy as part of the contract.

2. Tripwire — added test_registered_handlers_are_stateless. One deliberate deviation from the snippet, for robustness: it reads instance state through a small helper covering both __dict__ and __slots__, because bare vars(h) raises TypeError on a __slots__ handler — so a future slots-based handler would crash the test rather than be checked. I verified the guard actually fails on injected self.* state (not just passes vacuously).

Before pushing I also ran an independent per-handler thread-safety audit — you flagged that a byte-identical test won't reliably catch a race, which is exactly right. Result: all nine handlers are stateless on the extraction path today — no self.* writes, no class/module state mutated during extraction, fresh per-call file handles/hashers, per-call library objects (pyarrow / Pillow / pydicom / nibabel / wfdb), and imports resolved before workers start — so the parallel path is safe.

One caveat to note: the instance-state tripwire guards the narrowest case. The more dangerous future races carry no instance state and so are invisible to any introspective test — a class-level mutable attribute (e.g. a shared _cache = {} mutated in place), a module-level global, or a non-reentrant parser. I've written those into the docstring contract and a scoping note in the test. If you'd like, I can widen the tripwire to also reject class-level mutable containers — the tradeoff is that it would false-positive on read-only class-level lists (e.g. CSVHandler._TIMESTAMP_PARSERS) unless allowlisted, so I left the stricter check as your call rather than bake it in unasked.

Full suite green (283). Ready for another look whenever you have a moment.