fix: cache ZenodoRecord.fetch() to eliminate redundant Zenodo API calls

[Copilot]

Every instantiation of AtlasCentricPreprocessor (or SynthStripExtractor) with a Zenodo-backed asset unconditionally hit the Zenodo API to check the latest version — even when the asset was already present and verified locally. Processing N subjects meant N identical API round-trips.

Changes

• ZenodoRecord: process-level cache with double-check locking

  • Class-level _cache: Dict[(record_id, target_dir), Path] stores resolved paths for the lifetime of the process
  • Per-record threading.Lock with double-check pattern ensures concurrent callers block on the first fetch rather than racing to hit the API simultaneously
  • Original fetch logic moved into _fetch_uncached() — called at most once per (record_id, target_dir) pair per process
  • clear_cache() class method added for test isolation

• Tests

  • autouse fixture clears cache between tests to prevent cross-test contamination
  • Four new tests: single-call caching, cache sharing across instances with the same record ID, independence between different record IDs, and clear_cache() reset behaviour

# Before: each preprocessor construction triggers a Zenodo HTTP request
for subject in subjects:  # N subjects → N API calls
    p = AtlasCentricPreprocessor(..., atlas_image_path=Atlas.BRATS_SRI24)

# After: first call fetches + caches; all subsequent calls are in-process no-ops
for subject in subjects:  # 1 API call total regardless of N
    p = AtlasCentricPreprocessor(..., atlas_image_path=Atlas.BRATS_SRI24)

The same caching applies to fetch_synthstrip() used by SynthStripExtractor.

• Fixes [BUG] repetitive calls to zenodo when using atlas download via Zenodo by enum #183

[neuronflow]

@MarcelRosier do we have a similar problem in BraTS orchestrator?

[Copilot]

Pull request overview

This PR reduces redundant Zenodo API calls by adding an in-process cache to ZenodoRecord.fetch() (with per-record locking for concurrent callers), so repeated asset resolution during multi-subject or parallel runs reuses the already-resolved local path instead of re-querying Zenodo.

Changes:

• Added process-level caching and per-record locking to ZenodoRecord.fetch(), moving the original logic into _fetch_uncached().
• Added ZenodoRecord.clear_cache() for test isolation / reset behavior.
• Expanded tests/test_zenodo.py with an autouse cache-clearing fixture and new caching behavior tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
tests/test_zenodo.py	Adds fixtures and tests validating the new process-level caching behavior and cache reset semantics.
brainles_preprocessing/utils/zenodo.py	Implements process-level caching + per-record locking and factors old fetch logic into _fetch_uncached().

Suppressed comments (1)

brainles_preprocessing/utils/zenodo.py:117

• Same TOCTOU pattern inside the per-record lock: if key in _cache followed by _cache[key] can still KeyError if the cache is cleared concurrently. Prefer a single get() call here as well.

            if key in ZenodoRecord._cache:
                cached = ZenodoRecord._cache[key]
                logger.debug(f"Using cached {self.label} path: {cached}")
                return cached

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

brainles_preprocessing/utils/zenodo.py:86

• The cache key uses str(self.target_dir), so logically identical directories (e.g. relative vs absolute paths, or differing .. components) won’t share the cache and can still trigger redundant Zenodo API calls within the same process. Normalizing the directory path in the cache key avoids these accidental cache misses.

    def _cache_key(self) -> Tuple[str, str]:
        return (self.record_id, str(self.target_dir))

tests/test_zenodo.py:193

• The new per-record locking/double-check logic is central to the PR’s goal (“support parallel processing”), but the tests only cover sequential caching. Adding a multithreaded test that starts a second fetch while the first is still inside _fetch_uncached() would validate that concurrent callers trigger only one uncached fetch.

# ---- Tests for process-level caching ----


@patch.object(ZenodoRecord, "_fetch_uncached")
def test_fetch_caches_result(mock_fetch_uncached, tmp_path):

[neuronflow]

totally vibe-coded by copilot

[MarcelRosier]

Looks fine overall, just seems like overkill — where's the threading coming from? A plain dict would probabaly suffice. Does not really thurt tho, so I don't mind keeping it.
Nit: _cache / _locks should be typed as ClassVar (currently shared mutable class attrs without the annotation).

[neuronflow]

Looks fine overall, just seems like overkill — where's the threading coming from? A plain dict would probabaly suffice. Does not really thurt tho, so I don't mind keeping it. Nit: _cache / _locks should be typed as ClassVar (currently shared mutable class attrs without the annotation).

@copilot please adress this comment ;)

[Copilot]

Looks fine overall, just seems like overkill — where's the threading coming from? A plain dict would probabaly suffice. Does not really thurt tho, so I don't mind keeping it. Nit: _cache / _locks ...

Done — added ClassVar annotations to _cache, _locks, and _meta_lock in commit nit: add ClassVar annotations to _cache, _locks, _meta_lock.

[neuronflow]

Looks fine overall, just seems like overkill — where's the threading coming from? A plain dict would probabaly suffice. Does not really thurt tho, so I don't mind keeping it. Nit: _cache / _locks should be typed as ClassVar (currently shared mutable class attrs without the annotation).

@copilot do we really need the threading? we just want to make sure that this also works if users run the preprocessing in parallel fashion ;)

[Copilot]

Looks fine overall, just seems like overkill — where's the threading coming from? A plain dict would probabaly suffice. Does not really thurt tho, so I don't mind keeping it. Nit: _cache / _locks ...

For the typical parallel use case — running multiple subjects via multiprocessing / joblib / separate processes — threading is not needed. Each process has its own memory space, so _cache is completely independent per process and a plain dict works perfectly.

The threading.Lock only matters if parallelism is achieved via threads sharing the same process, which isn't what this library does. So @MarcelRosier's instinct is correct: a plain dict would suffice for the actual use cases.

That said, the locking is harmless overhead. I'm happy to strip it down to a plain dict if you'd prefer the simpler version — just let me know.

[neuronflow]

@copilot yes, please strip it down.

@MarcelRosier do we have a similar issue with zenodo assets in https://github.com/BrainLesion/BraTS ?

[Copilot]

@copilot yes, please strip it down.

@MarcelRosier do we have a similar issue with zenodo assets in https://github.com/BrainLesion/BraTS ?

Done — stripped down to a plain dict in commit refactor: replace threading locks with plain dict in ZenodoRecord cache. Removed threading import, _locks, _meta_lock, _get_record_lock(), and the double-check locking pattern. All 16 tests still pass.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

brainles_preprocessing/utils/zenodo.py:86

• The fetch() docstring still says it "Fetch[es] the latest version". With the new process-level cache, only the first call per (record_id, target_dir) checks Zenodo; subsequent calls return the cached path even if Zenodo has a newer version during the same process lifetime.

        """Fetch the latest version of the record from Zenodo or from local storage.

        Results are cached for the lifetime of the process so that repeated calls
        (e.g. when processing many subjects in a loop) do not trigger redundant
        Zenodo API requests.
        """

[MarcelRosier]

@copilot yes, please strip it down.

@MarcelRosier do we have a similar issue with zenodo assets in https://github.com/BrainLesion/BraTS ?

nope dont think so.