From 79f8a40d344f955b0313c84a2792c6eb7660b214 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Aug 2026 17:16:00 +0000 Subject: [PATCH 1/5] Initial plan From 1745c8d73f13ac5530929d89f0453d05eaae40c9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Aug 2026 17:21:10 +0000 Subject: [PATCH 2/5] fix: cache ZenodoRecord.fetch() results to avoid redundant API requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each call to fetch_atlases() or fetch_synthstrip() previously always issued a live HTTP request to the Zenodo API to check the latest version, even when the assets were already downloaded and verified in the same process. This caused redundant (N×) requests when processing many subjects in a loop or in parallel. Changes: - Add a process-level class-level cache (_cache dict, keyed by (record_id, target_dir)) to ZenodoRecord so that the resolved Path is returned immediately on subsequent calls without hitting Zenodo. - Use per-record threading.Lock with double-check locking so concurrent callers (e.g. parallel subject processing) wait for the first fetch to complete rather than all hitting the API simultaneously. - Extract the original fetch logic into _fetch_uncached() to keep concerns separate. - Add ZenodoRecord.clear_cache() class method for test isolation. - Add autouse fixture to clear the cache between tests. - Add four new tests covering: result caching, cache sharing across instances, independent caches for different record IDs, and clear_cache() behaviour. --- brainles_preprocessing/utils/zenodo.py | 56 +++++++++++++++++- tests/test_zenodo.py | 78 ++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/brainles_preprocessing/utils/zenodo.py b/brainles_preprocessing/utils/zenodo.py index b43f9d7..7638192 100644 --- a/brainles_preprocessing/utils/zenodo.py +++ b/brainles_preprocessing/utils/zenodo.py @@ -1,6 +1,7 @@ from __future__ import annotations import shutil +import threading import zipfile from io import BytesIO from pathlib import Path @@ -56,6 +57,13 @@ class ZenodoException(Exception): class ZenodoRecord: BASE_URL = "https://zenodo.org/api/records" + # Process-level cache: maps (record_id, target_dir) -> resolved Path + _cache: Dict[Tuple[str, str], Path] = {} + # Per-record locks to prevent redundant concurrent fetches of the same record + _locks: Dict[Tuple[str, str], threading.Lock] = {} + # Protects access to _locks itself + _meta_lock: threading.Lock = threading.Lock() + def __init__( self, record_id: str, @@ -66,8 +74,54 @@ def __init__( self.target_dir = target_dir self.label = label + @classmethod + def clear_cache(cls) -> None: + """Clear the process-level fetch cache. Primarily intended for testing.""" + with cls._meta_lock: + cls._cache.clear() + cls._locks.clear() + + def _cache_key(self) -> Tuple[str, str]: + return (self.record_id, str(self.target_dir)) + + def _get_record_lock(self) -> threading.Lock: + key = self._cache_key() + with ZenodoRecord._meta_lock: + if key not in ZenodoRecord._locks: + ZenodoRecord._locks[key] = threading.Lock() + return ZenodoRecord._locks[key] + def fetch(self) -> Path: - """Fetch the latest version of the record from Zenodo or from local storage.""" + """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 or in parallel) do not trigger + redundant Zenodo API requests. + """ + key = self._cache_key() + + # Fast path: return immediately if already resolved in this process + if key in ZenodoRecord._cache: + cached = ZenodoRecord._cache[key] + logger.debug(f"Using cached {self.label} path: {cached}") + return cached + + # Acquire per-record lock so that concurrent callers wait rather than + # all hitting the Zenodo API simultaneously. + lock = self._get_record_lock() + with lock: + # Double-check after acquiring the lock + if key in ZenodoRecord._cache: + cached = ZenodoRecord._cache[key] + logger.debug(f"Using cached {self.label} path: {cached}") + return cached + + result = self._fetch_uncached() + ZenodoRecord._cache[key] = result + return result + + def _fetch_uncached(self) -> Path: + """Perform the actual Zenodo check / download without consulting the cache.""" zenodo_response = self._get_metadata_and_archive_url() pattern = self._glob_pattern() diff --git a/tests/test_zenodo.py b/tests/test_zenodo.py index 7815b22..724cfe6 100644 --- a/tests/test_zenodo.py +++ b/tests/test_zenodo.py @@ -14,6 +14,14 @@ # ---- Fixtures ---- +@pytest.fixture(autouse=True) +def clear_zenodo_cache(): + """Ensure the process-level ZenodoRecord cache is clean before every test.""" + ZenodoRecord.clear_cache() + yield + ZenodoRecord.clear_cache() + + @pytest.fixture def dummy_metadata(): return { @@ -163,6 +171,76 @@ def test_fetch_replaces_old_version( mock_download.assert_called_once() +# ---- Tests for process-level caching ---- + + +@patch.object(ZenodoRecord, "_fetch_uncached") +def test_fetch_caches_result(mock_fetch_uncached, tmp_path): + """Second call to fetch() returns cached result without hitting _fetch_uncached.""" + expected_path = tmp_path / "123_v1.2.3" + mock_fetch_uncached.return_value = expected_path + + record = ZenodoRecord("123", tmp_path, "test") + + result1 = record.fetch() + result2 = record.fetch() + + assert result1 == expected_path + assert result2 == expected_path + # _fetch_uncached should only be called once despite two fetch() calls + mock_fetch_uncached.assert_called_once() + + +@patch.object(ZenodoRecord, "_fetch_uncached") +def test_fetch_cache_shared_across_instances(mock_fetch_uncached, tmp_path): + """Two separate ZenodoRecord instances with the same record_id share the cache.""" + expected_path = tmp_path / "123_v1.2.3" + mock_fetch_uncached.return_value = expected_path + + record1 = ZenodoRecord("123", tmp_path, "test") + record2 = ZenodoRecord("123", tmp_path, "other_label") + + result1 = record1.fetch() + result2 = record2.fetch() + + assert result1 == expected_path + assert result2 == expected_path + mock_fetch_uncached.assert_called_once() + + +@patch.object(ZenodoRecord, "_fetch_uncached") +def test_fetch_cache_different_records_independent(mock_fetch_uncached, tmp_path): + """Different record_ids each trigger their own fetch.""" + path_a = tmp_path / "aaa_v1.0.0" + path_b = tmp_path / "bbb_v2.0.0" + mock_fetch_uncached.side_effect = [path_a, path_b] + + record_a = ZenodoRecord("aaa", tmp_path, "a") + record_b = ZenodoRecord("bbb", tmp_path, "b") + + result_a = record_a.fetch() + result_b = record_b.fetch() + + assert result_a == path_a + assert result_b == path_b + assert mock_fetch_uncached.call_count == 2 + + +def test_clear_cache_resets_state(tmp_path): + """clear_cache() forces a fresh Zenodo check on the next fetch() call.""" + with patch.object(ZenodoRecord, "_fetch_uncached") as mock_fetch_uncached: + expected_path = tmp_path / "123_v1.2.3" + mock_fetch_uncached.return_value = expected_path + + record = ZenodoRecord("123", tmp_path, "test") + record.fetch() + assert mock_fetch_uncached.call_count == 1 + + ZenodoRecord.clear_cache() + record.fetch() + assert mock_fetch_uncached.call_count == 2 + + # ---- fetch_atlases and fetch_synthstrip ---- From cd9cde395ca66ae925ce6bd6f607a974958b9f21 Mon Sep 17 00:00:00 2001 From: neuronflow <7048826+neuronflow@users.noreply.github.com> Date: Sat, 1 Aug 2026 20:45:24 +0200 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- brainles_preprocessing/utils/zenodo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/brainles_preprocessing/utils/zenodo.py b/brainles_preprocessing/utils/zenodo.py index 7638192..31b02e5 100644 --- a/brainles_preprocessing/utils/zenodo.py +++ b/brainles_preprocessing/utils/zenodo.py @@ -101,8 +101,8 @@ def fetch(self) -> Path: key = self._cache_key() # Fast path: return immediately if already resolved in this process - if key in ZenodoRecord._cache: - cached = ZenodoRecord._cache[key] + cached = ZenodoRecord._cache.get(key) + if cached is not None: logger.debug(f"Using cached {self.label} path: {cached}") return cached From 180839bf60852bd305aca04fceab465a8ed91812 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:12:32 +0000 Subject: [PATCH 4/5] nit: add ClassVar annotations to _cache, _locks, _meta_lock --- brainles_preprocessing/utils/zenodo.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/brainles_preprocessing/utils/zenodo.py b/brainles_preprocessing/utils/zenodo.py index 31b02e5..08f2145 100644 --- a/brainles_preprocessing/utils/zenodo.py +++ b/brainles_preprocessing/utils/zenodo.py @@ -5,7 +5,7 @@ import zipfile from io import BytesIO from pathlib import Path -from typing import Dict, List, Tuple +from typing import ClassVar, Dict, List, Tuple import requests from loguru import logger @@ -58,11 +58,11 @@ class ZenodoRecord: BASE_URL = "https://zenodo.org/api/records" # Process-level cache: maps (record_id, target_dir) -> resolved Path - _cache: Dict[Tuple[str, str], Path] = {} + _cache: ClassVar[Dict[Tuple[str, str], Path]] = {} # Per-record locks to prevent redundant concurrent fetches of the same record - _locks: Dict[Tuple[str, str], threading.Lock] = {} + _locks: ClassVar[Dict[Tuple[str, str], threading.Lock]] = {} # Protects access to _locks itself - _meta_lock: threading.Lock = threading.Lock() + _meta_lock: ClassVar[threading.Lock] = threading.Lock() def __init__( self, From c871626a01de9fdf1a46df419441a19094afa57e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:23:16 +0000 Subject: [PATCH 5/5] refactor: replace threading locks with plain dict in ZenodoRecord cache --- brainles_preprocessing/utils/zenodo.py | 37 +++++--------------------- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/brainles_preprocessing/utils/zenodo.py b/brainles_preprocessing/utils/zenodo.py index 08f2145..fe7183f 100644 --- a/brainles_preprocessing/utils/zenodo.py +++ b/brainles_preprocessing/utils/zenodo.py @@ -1,7 +1,6 @@ from __future__ import annotations import shutil -import threading import zipfile from io import BytesIO from pathlib import Path @@ -59,10 +58,6 @@ class ZenodoRecord: # Process-level cache: maps (record_id, target_dir) -> resolved Path _cache: ClassVar[Dict[Tuple[str, str], Path]] = {} - # Per-record locks to prevent redundant concurrent fetches of the same record - _locks: ClassVar[Dict[Tuple[str, str], threading.Lock]] = {} - # Protects access to _locks itself - _meta_lock: ClassVar[threading.Lock] = threading.Lock() def __init__( self, @@ -77,48 +72,28 @@ def __init__( @classmethod def clear_cache(cls) -> None: """Clear the process-level fetch cache. Primarily intended for testing.""" - with cls._meta_lock: - cls._cache.clear() - cls._locks.clear() + cls._cache.clear() def _cache_key(self) -> Tuple[str, str]: return (self.record_id, str(self.target_dir)) - def _get_record_lock(self) -> threading.Lock: - key = self._cache_key() - with ZenodoRecord._meta_lock: - if key not in ZenodoRecord._locks: - ZenodoRecord._locks[key] = threading.Lock() - return ZenodoRecord._locks[key] - def fetch(self) -> Path: """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 or in parallel) do not trigger - redundant Zenodo API requests. + (e.g. when processing many subjects in a loop) do not trigger redundant + Zenodo API requests. """ key = self._cache_key() - # Fast path: return immediately if already resolved in this process cached = ZenodoRecord._cache.get(key) if cached is not None: logger.debug(f"Using cached {self.label} path: {cached}") return cached - # Acquire per-record lock so that concurrent callers wait rather than - # all hitting the Zenodo API simultaneously. - lock = self._get_record_lock() - with lock: - # Double-check after acquiring the lock - if key in ZenodoRecord._cache: - cached = ZenodoRecord._cache[key] - logger.debug(f"Using cached {self.label} path: {cached}") - return cached - - result = self._fetch_uncached() - ZenodoRecord._cache[key] = result - return result + result = self._fetch_uncached() + ZenodoRecord._cache[key] = result + return result def _fetch_uncached(self) -> Path: """Perform the actual Zenodo check / download without consulting the cache."""