From 534377d6e5c73805b12b021e781603882ee47dcb Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 14:55:11 -0700 Subject: [PATCH 01/20] Allow the worker count to be capped per call and pinned in workers Split the memoized lookups so that a caller can supply an upper bound, and add a flag that a pool worker sets to report a single worker. Co-Authored-By: Claude Opus 5 (1M context) --- python/lsst/resources/utils.py | 86 ++++++++++++++++++++++++++-------- tests/test_utils.py | 65 +++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 20 deletions(-) create mode 100644 tests/test_utils.py diff --git a/python/lsst/resources/utils.py b/python/lsst/resources/utils.py index e29f687b..0812b209 100644 --- a/python/lsst/resources/utils.py +++ b/python/lsst/resources/utils.py @@ -34,9 +34,10 @@ # posix means posix and only determine explicitly in the non-posix case. OS_ROOT_PATH = posixpath.sep if IS_POSIX else Path().resolve().root -# Maximum number of worker threads for parallelized operations. -# If greater than 10, be aware that this number has to be consistent -# with connection pool sizing (for example in urllib3). +# Default upper bound on the number of workers for parallelized operations. +# Subclasses of ResourcePath override this for schemes that have no connection +# pool to contend with. Backends that do have one size that pool from the +# worker count, so they need no separate coordination here. MAX_WORKERS = 10 log = logging.getLogger(__name__) @@ -243,29 +244,74 @@ def _get_int_env_var(env_var: str) -> int | None: return int_value +# True in processes started as pool workers. Only ever written by +# _init_pool_worker, which runs once per worker process, so the reads need no +# locking. +_IS_POOL_WORKER = False + + +def _init_pool_worker() -> None: + """Mark this process as a pool worker. + + Notes + ----- + Used as the ``initializer`` of a `~concurrent.futures.ProcessPoolExecutor` + so that parallel operations running inside a worker do not spawn workers + of their own. Must not be used with a thread pool, since threads share + this global with the process that created them. + """ + global _IS_POOL_WORKER + _IS_POOL_WORKER = True + + +@cache +def _get_configured_num_workers() -> int | None: + """Return the explicitly requested number of workers. + + Returns + ------- + num : `int` or `None` + Value of the ``LSST_RESOURCES_NUM_WORKERS`` environment variable, or + `None` if it is unset or unparsable. + """ + return _get_int_env_var("LSST_RESOURCES_NUM_WORKERS") + + @cache -def _get_num_workers() -> int: - f"""Calculate the number of workers to use. +def _get_default_num_workers() -> int: + """Return the number of workers implied by the available CPUs. Returns ------- num : `int` - The number of workers to use. Will use the value of the - ``LSST_RESOURCES_NUM_WORKERS`` environment variable if set. Will fall - back to using the CPU count (plus 2) but capped at {MAX_WORKERS}. + The CPU count plus two. Uncapped. """ - num_workers: int | None = None - num_workers = _get_int_env_var("LSST_RESOURCES_NUM_WORKERS") + # CPU_LIMIT is used on nublado. + cpu_limit = _get_int_env_var("CPU_LIMIT") or multiprocessing.cpu_count() + return cpu_limit + 2 - # If someone is explicitly specifying a number, let them use that number. - if num_workers is not None: - return num_workers - if num_workers is None: - # CPU_LIMIT is used on nublado. - cpu_limit = _get_int_env_var("CPU_LIMIT") or multiprocessing.cpu_count() - if cpu_limit is not None: - num_workers = cpu_limit + 2 +def _get_num_workers(max_workers: int = MAX_WORKERS) -> int: + """Calculate the number of workers to use. - # But don't ever return more than the maximum allowed. - return min([num_workers, MAX_WORKERS]) + Parameters + ---------- + max_workers : `int`, optional + Upper bound to apply to the calculated default. Ignored when the + number of workers has been requested explicitly. + + Returns + ------- + num : `int` + The number of workers to use. A pool worker always reports one, so + that nested parallel operations do not multiply. Otherwise the value + of ``$LSST_RESOURCES_NUM_WORKERS`` is used if set, and the CPU count + plus two bounded by ``max_workers`` if not. + """ + if _IS_POOL_WORKER: + return 1 + configured = _get_configured_num_workers() + if configured is not None: + # An explicit request is honored without capping. + return configured + return min(_get_default_num_workers(), max_workers) diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 00000000..4b284eca --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,65 @@ +# This file is part of lsst-resources. +# +# Developed for the LSST Data Management System. +# This product includes software developed by the LSST Project +# (https://www.lsst.org). +# See the COPYRIGHT file at the top-level directory of this distribution +# for details of code ownership. +# +# Use of this source code is governed by a 3-clause BSD-style +# license that can be found in the LICENSE file. + +import os +import unittest +import unittest.mock + +from lsst.resources.utils import ( + MAX_WORKERS, + _get_configured_num_workers, + _get_default_num_workers, + _get_num_workers, +) + + +def _clear_worker_caches() -> None: + """Discard memoized worker-count lookups.""" + _get_configured_num_workers.cache_clear() + _get_default_num_workers.cache_clear() + + +class NumWorkersTestCase(unittest.TestCase): + """Tests for the worker-count calculation.""" + + def setUp(self) -> None: + _clear_worker_caches() + + def tearDown(self) -> None: + _clear_worker_caches() + + @unittest.mock.patch.dict(os.environ, {}, clear=False) + def test_default_is_capped(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + self.assertLessEqual(_get_num_workers(), MAX_WORKERS) + self.assertEqual(_get_num_workers(2), 2) + + @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_NUM_WORKERS": "99"}) + def test_explicit_request_bypasses_cap(self) -> None: + _clear_worker_caches() + self.assertEqual(_get_num_workers(), 99) + self.assertEqual(_get_num_workers(2), 99) + + @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_NUM_WORKERS": "99"}) + @unittest.mock.patch("lsst.resources.utils._IS_POOL_WORKER", True) + def test_pool_worker_uses_one_worker(self) -> None: + _clear_worker_caches() + self.assertEqual(_get_num_workers(), 1) + self.assertEqual(_get_num_workers(99), 1) + + def test_docstring_is_present(self) -> None: + # An f-string in the leading position is not a docstring. + self.assertIsNotNone(_get_num_workers.__doc__) + + +if __name__ == "__main__": + unittest.main() From d100659a915b6d17fb7462ac3f896c11d05f1460 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 14:59:30 -0700 Subject: [PATCH 02/20] Mark pool worker processes instead of patching the environment The worker count for a subprocess is now set by a pool initializer, so it no longer depends on whether the parent had already calculated its own count and behaves the same under the fork and spawn start methods. Also stop dropping the caller's num_workers argument to mexists() when a process pool is in use. Co-Authored-By: Claude Opus 5 (1M context) --- python/lsst/resources/_resourcePath.py | 77 ++++++++++---------------- python/lsst/resources/s3.py | 13 +---- tests/test_utils.py | 55 ++++++++++++++++++ 3 files changed, 86 insertions(+), 59 deletions(-) diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 692d8142..adee7732 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -45,7 +45,7 @@ from typing import Any, Literal, NamedTuple, overload from ._resourceHandles._baseResourceHandle import ResourceHandleProtocol -from .utils import _get_num_workers, get_tempdir +from .utils import _get_num_workers, _init_pool_worker, get_tempdir if TYPE_CHECKING: from .utils import TransactionProtocol @@ -112,28 +112,31 @@ def _get_executor_class() -> _EXECUTOR_TYPE: return _POOL_EXECUTOR_CLASS -@contextlib.contextmanager -def _patch_environ(new_values: dict[str, str]) -> Generator[None]: - """Patch os.environ temporarily using the supplied values. +def _make_pool_executor(pool_executor_class: _EXECUTOR_TYPE, max_workers: int) -> concurrent.futures.Executor: + """Create a pool executor of the requested type and size. Parameters ---------- - new_values : `dict` [ `str`, `str` ] - New values to be stored in the environment. - """ - old_values: dict[str, str] = {} - for k, v in new_values.items(): - if k in os.environ: - old_values[k] = os.environ[k] - os.environ[k] = v + pool_executor_class : `type` [ `concurrent.futures.Executor` ] + Type of executor pool to create. + max_workers : `int` + Number of workers the pool should use. + + Returns + ------- + executor : `concurrent.futures.Executor` + The new executor. - try: - yield - finally: - for k in new_values: - del os.environ[k] - if k in old_values: - os.environ[k] = old_values[k] + Notes + ----- + A process pool marks each of its workers so that parallel operations + running inside a worker use a single worker of their own. A thread pool + must not be marked, since its threads share that state with the process + that created them. + """ + if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): + return pool_executor_class(max_workers=max_workers, initializer=_init_pool_worker) + return pool_executor_class(max_workers=max_workers) @dataclasses.dataclass(frozen=True) @@ -1035,13 +1038,7 @@ def _mexists( Mapping of original URI to boolean indicating existence. """ pool_executor_class = _get_executor_class() - if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - # Patch the environment to make it think there is only one worker - # for each subprocess. - with _patch_environ({"LSST_RESOURCES_NUM_WORKERS": "1"}): - return cls._mexists_pool(pool_executor_class, uris) - else: - return cls._mexists_pool(pool_executor_class, uris, num_workers=num_workers) + return cls._mexists_pool(pool_executor_class, uris, num_workers=num_workers) @classmethod def _mexists_pool( @@ -1072,7 +1069,7 @@ def _mexists_pool( Mapping of original URI to boolean indicating existence. """ max_workers = num_workers if num_workers is not None else _get_num_workers() - with pool_executor_class(max_workers=max_workers) as exists_executor: + with _make_pool_executor(pool_executor_class, max_workers) as exists_executor: future_exists = {exists_executor.submit(uri.exists): uri for uri in uris} results: dict[ResourcePath, bool] = {} @@ -1124,18 +1121,6 @@ def mtransfer( is `True`, this will only be returned if there are no errors. """ pool_executor_class = _get_executor_class() - if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - # Patch the environment to make it think there is only one worker - # for each subprocess. - with _patch_environ({"LSST_RESOURCES_NUM_WORKERS": "1"}): - return cls._mtransfer( - pool_executor_class, - transfer, - from_to, - overwrite=overwrite, - transaction=transaction, - do_raise=do_raise, - ) return cls._mtransfer( pool_executor_class, transfer, @@ -1182,7 +1167,8 @@ def _mtransfer( A dict of all the transfer attempts with a value indicating whether the transfer succeeded for the target URI. """ - with pool_executor_class(max_workers=_get_num_workers()) as transfer_executor: + max_workers = _get_num_workers() + with _make_pool_executor(pool_executor_class, max_workers) as transfer_executor: future_transfers = { transfer_executor.submit( to_uri.transfer_from, @@ -1261,14 +1247,7 @@ def mremove( @classmethod def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResult]: """Remove multiple URIs using futures.""" - pool_executor_class = _get_executor_class() - if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - # Patch the environment to make it think there is only one worker - # for each subprocess. - with _patch_environ({"LSST_RESOURCES_NUM_WORKERS": "1"}): - return cls._mremove_pool(pool_executor_class, uris) - else: - return cls._mremove_pool(pool_executor_class, uris) + return cls._mremove_pool(_get_executor_class(), uris) @classmethod def _mremove_pool( @@ -1281,7 +1260,7 @@ def _mremove_pool( """Remove URIs using a futures pool.""" max_workers = num_workers if num_workers is not None else _get_num_workers() results: dict[ResourcePath, MBulkResult] = {} - with pool_executor_class(max_workers=max_workers) as remove_executor: + with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = {remove_executor.submit(uri.remove): uri for uri in uris} for future in concurrent.futures.as_completed(future_remove): try: diff --git a/python/lsst/resources/s3.py b/python/lsst/resources/s3.py index 276d54c8..5ca4a9c0 100644 --- a/python/lsst/resources/s3.py +++ b/python/lsst/resources/s3.py @@ -41,7 +41,7 @@ ResourceInfo, ResourcePath, _get_executor_class, - _patch_environ, + _make_pool_executor, ) from .s3utils import ( _get_s3_connection_parameters, @@ -295,14 +295,7 @@ def _mremove_select(cls, chunks: list[tuple[ResourcePath, ...]]) -> dict[Resourc if len(chunks) == 1: # Do the removal directly without futures. return cls._delete_objects_wrapper(chunks[0]) - pool_executor_class = _get_executor_class() - if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - # Patch the environment to make it think there is only one worker - # for each subprocess. - with _patch_environ({"LSST_RESOURCES_NUM_WORKERS": "1"}): - return cls._mremove_with_pool(pool_executor_class, chunks) - else: - return cls._mremove_with_pool(pool_executor_class, chunks) + return cls._mremove_with_pool(_get_executor_class(), chunks) @classmethod def _mremove_with_pool( @@ -316,7 +309,7 @@ def _mremove_with_pool( # No need to make more workers than we have chunks. max_workers = num_workers if num_workers is not None else min(len(chunks), _get_num_workers()) results: dict[ResourcePath, MBulkResult] = {} - with pool_executor_class(max_workers=max_workers) as remove_executor: + with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = { remove_executor.submit(cls._delete_objects_wrapper, chunk): i for i, chunk in enumerate(chunks) diff --git a/tests/test_utils.py b/tests/test_utils.py index 4b284eca..5df245ed 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -9,15 +9,20 @@ # Use of this source code is governed by a 3-clause BSD-style # license that can be found in the LICENSE file. +import concurrent.futures +import multiprocessing import os import unittest import unittest.mock +from typing import Any +from lsst.resources._resourcePath import _make_pool_executor from lsst.resources.utils import ( MAX_WORKERS, _get_configured_num_workers, _get_default_num_workers, _get_num_workers, + _init_pool_worker, ) @@ -61,5 +66,55 @@ def test_docstring_is_present(self) -> None: self.assertIsNotNone(_get_num_workers.__doc__) +class PoolExecutorTestCase(unittest.TestCase): + """Tests for worker-count propagation into pool executors.""" + + def setUp(self) -> None: + _clear_worker_caches() + + def tearDown(self) -> None: + _clear_worker_caches() + + def test_process_worker_reports_one_worker(self) -> None: + # The fork start method is the one where a child inherits the parent's + # memoized state, so both methods must be checked. + for method in ("fork", "spawn"): + with self.subTest(start_method=method): + parent_before = _get_num_workers() + context = multiprocessing.get_context(method) + with concurrent.futures.ProcessPoolExecutor( + max_workers=2, + mp_context=context, + initializer=_init_pool_worker, + ) as executor: + # The callable has to come from an installed module. The + # spawn start method pickles it by reference and the child + # process cannot import this test module. + observed = list(executor.map(_get_num_workers, [MAX_WORKERS] * 4)) + self.assertEqual(observed, [1, 1, 1, 1]) + self.assertEqual(_get_num_workers(), parent_before) + + def test_thread_pool_does_not_mark_the_parent(self) -> None: + parent_before = _get_num_workers() + executor = _make_pool_executor(concurrent.futures.ThreadPoolExecutor, 2) + with executor: + observed = list(executor.map(_get_num_workers, [MAX_WORKERS] * 4)) + self.assertEqual(observed, [parent_before] * 4) + self.assertEqual(_get_num_workers(), parent_before) + + def test_process_pool_receives_the_requested_size(self) -> None: + recorded: list[int] = [] + + class _RecordingExecutor(concurrent.futures.ProcessPoolExecutor): + def __init__(self, max_workers: int, **kwargs: Any) -> None: + recorded.append(max_workers) + super().__init__(max_workers=max_workers, **kwargs) + + # A cold cache must not cause the parent pool to shrink to one. + _clear_worker_caches() + _make_pool_executor(_RecordingExecutor, 7).shutdown() + self.assertEqual(recorded, [7]) + + if __name__ == "__main__": unittest.main() From 9191f47de4ef52539b88b2c14df67c50a40de7b1 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:00:30 -0700 Subject: [PATCH 03/20] Bound the worker count per scheme rather than globally Schemes backed by a connection pool keep the modest default; file URIs hold no pool and so raise their bound. Co-Authored-By: Claude Opus 5 (1M context) --- python/lsst/resources/_resourcePath.py | 20 +++++++++++------- python/lsst/resources/file.py | 4 ++++ tests/test_utils.py | 28 ++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index adee7732..fa64bacd 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -45,7 +45,7 @@ from typing import Any, Literal, NamedTuple, overload from ._resourceHandles._baseResourceHandle import ResourceHandleProtocol -from .utils import _get_num_workers, _init_pool_worker, get_tempdir +from .utils import MAX_WORKERS, _get_num_workers, _init_pool_worker, get_tempdir if TYPE_CHECKING: from .utils import TransactionProtocol @@ -230,6 +230,13 @@ class ResourcePath: # numpydoc ignore=PR02 isLocal = False """If `True` this URI refers to a local file.""" + _max_workers: int = MAX_WORKERS + """Upper bound on workers for parallel operations on this scheme. + + Schemes backed by a connection pool keep this modest because the pool is + sized to match it; schemes with no pool can raise it. + """ + # This is not an ABC with abstract methods because the __new__ being # a factory confuses mypy such that it assumes that every constructor # returns a ResourcePath and then determines that all the abstract methods @@ -999,10 +1006,9 @@ def mexists( uris : iterable of `ResourcePath` The URIs to test. num_workers : `int` or `None`, optional - The number of parallel workers to use when checking for existence - If `None`, the default value will be taken from the environment. - If this number is higher than the default and a thread pool is - used, there may not be enough cached connections available. + The number of parallel workers to use when checking for existence. + If `None`, the default value will be taken from the environment + and bounded by the limit for this scheme. Returns ------- @@ -1068,7 +1074,7 @@ def _mexists_pool( existence : `dict` of [`ResourcePath`, `bool`] Mapping of original URI to boolean indicating existence. """ - max_workers = num_workers if num_workers is not None else _get_num_workers() + max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) with _make_pool_executor(pool_executor_class, max_workers) as exists_executor: future_exists = {exists_executor.submit(uri.exists): uri for uri in uris} @@ -1258,7 +1264,7 @@ def _mremove_pool( num_workers: int | None = None, ) -> dict[ResourcePath, MBulkResult]: """Remove URIs using a futures pool.""" - max_workers = num_workers if num_workers is not None else _get_num_workers() + max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) results: dict[ResourcePath, MBulkResult] = {} with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = {remove_executor.submit(uri.remove): uri for uri in uris} diff --git a/python/lsst/resources/file.py b/python/lsst/resources/file.py index 5bef1b73..449fcd86 100644 --- a/python/lsst/resources/file.py +++ b/python/lsst/resources/file.py @@ -84,6 +84,10 @@ class FileResourcePath(ResourcePath): # By definition refers to a local file isLocal = True + # Local removal and stat calls are latency bound and hold no connection + # pool, so more workers help well past the default bound. + _max_workers: int = 32 + @property def ospath(self) -> str: """Path component of the URI localized to current OS. diff --git a/tests/test_utils.py b/tests/test_utils.py index 5df245ed..b4ab8dd8 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -16,7 +16,10 @@ import unittest.mock from typing import Any +from lsst.resources import ResourcePath from lsst.resources._resourcePath import _make_pool_executor +from lsst.resources.file import FileResourcePath +from lsst.resources.s3 import S3ResourcePath from lsst.resources.utils import ( MAX_WORKERS, _get_configured_num_workers, @@ -116,5 +119,30 @@ def __init__(self, max_workers: int, **kwargs: Any) -> None: self.assertEqual(recorded, [7]) +class WorkerCapTestCase(unittest.TestCase): + """Tests for per-scheme worker caps.""" + + def setUp(self) -> None: + _clear_worker_caches() + + def tearDown(self) -> None: + _clear_worker_caches() + + def test_local_scheme_allows_more_workers(self) -> None: + self.assertGreater(FileResourcePath._max_workers, S3ResourcePath._max_workers) + self.assertEqual(ResourcePath._max_workers, MAX_WORKERS) + + @unittest.mock.patch.dict(os.environ, {}, clear=False) + def test_cap_limits_the_default(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + self.assertLessEqual(_get_num_workers(S3ResourcePath._max_workers), S3ResourcePath._max_workers) + + @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_NUM_WORKERS": "99"}) + def test_explicit_request_overrides_the_scheme_cap(self) -> None: + _clear_worker_caches() + self.assertEqual(_get_num_workers(S3ResourcePath._max_workers), 99) + + if __name__ == "__main__": unittest.main() From b950a1ee15c107634084086ad541367f1c1625a0 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:02:06 -0700 Subject: [PATCH 04/20] Send batches of URIs to each removal worker Each batch reports every URI independently, so one failure does not prevent the removal of the URIs that follow it in the same batch. Co-Authored-By: Claude Opus 5 (1M context) --- python/lsst/resources/_resourcePath.py | 73 +++++++++++++++++++++++--- tests/test_file.py | 45 ++++++++++++++++ 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index fa64bacd..3baaa35f 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -21,6 +21,7 @@ import io import locale import logging +import math import os import posixpath import re @@ -44,6 +45,8 @@ from collections.abc import Generator, Iterable, Iterator from typing import Any, Literal, NamedTuple, overload +from lsst.utils.iteration import chunk_iterable + from ._resourceHandles._baseResourceHandle import ResourceHandleProtocol from .utils import MAX_WORKERS, _get_num_workers, _init_pool_worker, get_tempdir @@ -59,6 +62,10 @@ # Precomputed escaped hash ESCAPED_HASH = urllib.parse.quote("#") +# Chunks to create per worker when batching bulk operations. Oversubscribing +# keeps one slow chunk from stalling a worker for the rest of the run. +CHUNKS_PER_WORKER = 4 + class MBulkResult(NamedTuple): """Report on a bulk operation.""" @@ -1255,6 +1262,52 @@ def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResul """Remove multiple URIs using futures.""" return cls._mremove_pool(_get_executor_class(), uris) + @staticmethod + def _chunk_for_removal(uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: + """Split URIs into batches sized for the given number of workers. + + Parameters + ---------- + uris : `list` [ `ResourcePath` ] + The URIs to split. + max_workers : `int` + Number of workers the batches will be spread across. + + Returns + ------- + chunks : `list` [ `tuple` [ `ResourcePath`, ... ] ] + The batches. Empty if ``uris`` is empty. + """ + if not uris: + return [] + chunk_size = max(1, math.ceil(len(uris) / (max_workers * CHUNKS_PER_WORKER))) + return list(chunk_iterable(uris, chunk_size=chunk_size)) + + @classmethod + def _remove_chunk(cls, uris: tuple[ResourcePath, ...]) -> dict[ResourcePath, MBulkResult]: + """Remove a batch of URIs, reporting each result independently. + + Parameters + ---------- + uris : `tuple` [ `ResourcePath`, ... ] + The URIs to remove. + + Returns + ------- + results : `dict` [ `ResourcePath`, `MBulkResult` ] + An entry for every URI in ``uris``. A URI that cannot be removed + does not prevent the removal of the URIs after it. + """ + results: dict[ResourcePath, MBulkResult] = {} + for uri in uris: + try: + uri.remove() + except Exception as e: + results[uri] = MBulkResult(False, e) + else: + results[uri] = MBulkResult(True, None) + return results + @classmethod def _mremove_pool( cls, @@ -1264,19 +1317,25 @@ def _mremove_pool( num_workers: int | None = None, ) -> dict[ResourcePath, MBulkResult]: """Remove URIs using a futures pool.""" + uri_list = list(uris) max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) + chunks = cls._chunk_for_removal(uri_list, max_workers) + if not chunks: + return {} + # No need for more workers than there are chunks to give them. + max_workers = min(max_workers, len(chunks)) + results: dict[ResourcePath, MBulkResult] = {} with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: - future_remove = {remove_executor.submit(uri.remove): uri for uri in uris} + future_remove = {remove_executor.submit(cls._remove_chunk, chunk): chunk for chunk in chunks} for future in concurrent.futures.as_completed(future_remove): try: - future.result() + results.update(future.result()) except Exception as e: - removed = MBulkResult(False, e) - else: - removed = MBulkResult(True, None) - uri = future_remove[future] - results[uri] = removed + # The chunk failed as a whole, for example because a + # worker died. + for uri in future_remove[future]: + results[uri] = MBulkResult(False, e) return results def isabs(self) -> bool: diff --git a/tests/test_file.py b/tests/test_file.py index 45a31e62..62208756 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -18,7 +18,9 @@ import urllib.parse from lsst.resources import ResourceInfo, ResourcePath, ResourcePathExpression +from lsst.resources.file import FileResourcePath from lsst.resources.tests import GenericReadWriteTestCase, GenericTestCase +from lsst.resources.utils import makeTestTempDir, removeTestTempDir TESTDIR = os.path.abspath(os.path.dirname(__file__)) @@ -267,6 +269,49 @@ def test_mtransfer_process(self) -> None: super().test_mtransfer() +class RemoveChunkTestCase(unittest.TestCase): + """Tests for batched removal.""" + + def setUp(self) -> None: + self.tmpdir = ResourcePath(makeTestTempDir(TESTDIR), forceDirectory=True) + + def tearDown(self) -> None: + removeTestTempDir(self.tmpdir.ospath) + + def test_failure_does_not_abandon_the_rest_of_the_chunk(self) -> None: + uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] + for uri in uris: + uri.write(b"") + # Remove one out from under the batch so that its own removal raises. + uris[1].remove() + + results = FileResourcePath._remove_chunk(tuple(uris)) + + self.assertEqual(len(results), 5) + self.assertFalse(results[uris[1]].success) + self.assertIsInstance(results[uris[1]].exception, FileNotFoundError) + for uri in (uris[0], uris[2], uris[3], uris[4]): + self.assertTrue(results[uri].success, f"{uri} should have been removed") + self.assertFalse(uri.exists()) + + def test_chunk_sizes(self) -> None: + uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] + # Fewer URIs than chunk slots gives one URI per chunk. + chunks = FileResourcePath._chunk_for_removal(uris, 32) + self.assertEqual(len(chunks), 5) + self.assertTrue(all(len(c) == 1 for c in chunks)) + + # More URIs than chunk slots gives evenly sized chunks. + chunks = FileResourcePath._chunk_for_removal(uris, 1) + self.assertEqual([len(c) for c in chunks], [2, 2, 1]) + + # An empty input yields no chunks at all. + self.assertEqual(FileResourcePath._chunk_for_removal([], 4), []) + + def test_empty_removal_is_a_no_op(self) -> None: + self.assertEqual(ResourcePath.mremove([]), {}) + + @contextlib.contextmanager def _override_umask(temp_umask): old = os.umask(temp_umask) From 386484b6a64244ba6ecf669809deaf622d64c910 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:02:20 -0700 Subject: [PATCH 05/20] Add changelog fragments for DM-56097 Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.bugfix.rst | 3 +++ doc/changes/DM-56097.perf.rst | 2 ++ 2 files changed, 5 insertions(+) create mode 100644 doc/changes/DM-56097.bugfix.rst create mode 100644 doc/changes/DM-56097.perf.rst diff --git a/doc/changes/DM-56097.bugfix.rst b/doc/changes/DM-56097.bugfix.rst new file mode 100644 index 00000000..4794977a --- /dev/null +++ b/doc/changes/DM-56097.bugfix.rst @@ -0,0 +1,3 @@ +Fixed the number of workers used by bulk operations when ``$LSST_RESOURCES_EXECUTOR`` is set to ``process``. +The worker count for a subprocess is now set by a pool initializer rather than through the environment, so it no longer depends on whether the parent process had already calculated its own worker count, and it behaves the same under the ``fork`` and ``spawn`` start methods. +``ResourcePath.mexists()`` now honors an explicit ``num_workers`` argument when a process pool is in use. diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst new file mode 100644 index 00000000..5bab913d --- /dev/null +++ b/doc/changes/DM-56097.perf.rst @@ -0,0 +1,2 @@ +``ResourcePath.mremove()`` now sends a batch of URIs to each worker rather than submitting one task per URI. +The upper bound on workers is now set per scheme, and ``file`` URIs default to a higher bound because they use no connection pool. From a8505124b4e9698fe6d3fb7fed069fcef32a6bc3 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:16:54 -0700 Subject: [PATCH 06/20] Keep the default worker bound for file URIs Benchmarking local removal showed that extra threads add contention rather than throughput, because unlink on a local filesystem is not latency bound. The per-scheme bound remains available for a scheme that measures otherwise. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.perf.rst | 2 +- python/lsst/resources/file.py | 4 ---- tests/test_file.py | 37 ++++++++++++++++++++++++++++++++++- tests/test_utils.py | 12 ++++++++++-- 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 5bab913d..e496526c 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,2 +1,2 @@ ``ResourcePath.mremove()`` now sends a batch of URIs to each worker rather than submitting one task per URI. -The upper bound on workers is now set per scheme, and ``file`` URIs default to a higher bound because they use no connection pool. +The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. diff --git a/python/lsst/resources/file.py b/python/lsst/resources/file.py index 449fcd86..5bef1b73 100644 --- a/python/lsst/resources/file.py +++ b/python/lsst/resources/file.py @@ -84,10 +84,6 @@ class FileResourcePath(ResourcePath): # By definition refers to a local file isLocal = True - # Local removal and stat calls are latency bound and hold no connection - # pool, so more workers help well past the default bound. - _max_workers: int = 32 - @property def ospath(self) -> str: """Path component of the URI localized to current OS. diff --git a/tests/test_file.py b/tests/test_file.py index 62208756..e30dabc2 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -9,6 +9,7 @@ # Use of this source code is governed by a 3-clause BSD-style # license that can be found in the LICENSE file. +import concurrent.futures import contextlib import datetime import os @@ -16,11 +17,24 @@ import unittest import unittest.mock import urllib.parse +from typing import Any from lsst.resources import ResourceInfo, ResourcePath, ResourcePathExpression from lsst.resources.file import FileResourcePath from lsst.resources.tests import GenericReadWriteTestCase, GenericTestCase -from lsst.resources.utils import makeTestTempDir, removeTestTempDir +from lsst.resources.utils import ( + _get_configured_num_workers, + _get_default_num_workers, + makeTestTempDir, + removeTestTempDir, +) + + +def _clear_worker_caches() -> None: + """Discard memoized worker-count lookups.""" + _get_configured_num_workers.cache_clear() + _get_default_num_workers.cache_clear() + TESTDIR = os.path.abspath(os.path.dirname(__file__)) @@ -311,6 +325,27 @@ def test_chunk_sizes(self) -> None: def test_empty_removal_is_a_no_op(self) -> None: self.assertEqual(ResourcePath.mremove([]), {}) + @unittest.mock.patch.dict(os.environ, {}, clear=False) + @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + def test_scheme_cap_sizes_the_pool(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + recorded: list[int] = [] + + class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): + def __init__(self, max_workers: int, **kwargs: Any) -> None: + recorded.append(max_workers) + super().__init__(max_workers=max_workers, **kwargs) + + uris = [self.tmpdir.join(f"f{n}.txt") for n in range(20)] + for uri in uris: + uri.write(b"") + + results = FileResourcePath._mremove_pool(_RecordingExecutor, uris) + + self.assertEqual(recorded, [3]) + self.assertTrue(all(r.success for r in results.values())) + @contextlib.contextmanager def _override_umask(temp_umask): diff --git a/tests/test_utils.py b/tests/test_utils.py index b4ab8dd8..f2c08d0b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -128,9 +128,17 @@ def setUp(self) -> None: def tearDown(self) -> None: _clear_worker_caches() - def test_local_scheme_allows_more_workers(self) -> None: - self.assertGreater(FileResourcePath._max_workers, S3ResourcePath._max_workers) + def test_schemes_share_the_default_cap(self) -> None: self.assertEqual(ResourcePath._max_workers, MAX_WORKERS) + self.assertEqual(FileResourcePath._max_workers, MAX_WORKERS) + self.assertEqual(S3ResourcePath._max_workers, MAX_WORKERS) + + @unittest.mock.patch.dict(os.environ, {}, clear=False) + @unittest.mock.patch.object(FileResourcePath, "_max_workers", 2) + def test_an_overridden_cap_is_honored(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + self.assertEqual(_get_num_workers(FileResourcePath._max_workers), 2) @unittest.mock.patch.dict(os.environ, {}, clear=False) def test_cap_limits_the_default(self) -> None: From 2ecfa9471392a344ed56f3bf4c020efc52c6531f Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:33:37 -0700 Subject: [PATCH 07/20] Reuse process pools between bulk operations Under the spawn start method each worker pays a full interpreter startup, and the cost grows with the number of modules the calling process has imported. A pool is now kept alive and handed to later calls, and is discarded if it breaks. Thread pools are still created per call, since they are cheap and holding one open would keep its threads alive for no benefit. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.perf.rst | 2 + python/lsst/resources/_resourcePath.py | 94 +++++++++++++++++++++++++- python/lsst/resources/s3.py | 4 +- tests/test_utils.py | 64 +++++++++++++++++- 4 files changed, 158 insertions(+), 6 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index e496526c..0ebe0266 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,2 +1,4 @@ ``ResourcePath.mremove()`` now sends a batch of URIs to each worker rather than submitting one task per URI. +Process pools are now reused between bulk operations instead of being created and destroyed for each call. +Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that removes many artifacts. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 3baaa35f..9c6970b4 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -13,6 +13,7 @@ __all__ = ("ResourceInfo", "ResourcePath", "ResourcePathExpression") +import atexit import concurrent.futures import contextlib import copy @@ -146,6 +147,93 @@ def _make_pool_executor(pool_executor_class: _EXECUTOR_TYPE, max_workers: int) - return pool_executor_class(max_workers=max_workers) +# Process pools, keyed by executor class and worker count. Starting one costs +# a full interpreter startup per worker under the spawn start method, and the +# cost scales with how much the parent process has imported, so a pool is kept +# alive for reuse by later bulk operations. Thread pools are not cached; they +# cost almost nothing to create and holding one open would keep its threads +# alive for no benefit. +_POOL_EXECUTOR_CACHE: dict[tuple[_EXECUTOR_TYPE, int], concurrent.futures.Executor] = {} + + +def _clear_pool_executor_cache() -> None: + """Shut down and forget every cached pool executor.""" + while _POOL_EXECUTOR_CACHE: + _, executor = _POOL_EXECUTOR_CACHE.popitem() + executor.shutdown(wait=True) + + +def _forget_pool_executor_cache() -> None: + """Forget every cached pool executor without shutting it down. + + Notes + ----- + For use in a child process after a fork, where the inherited executors + refer to worker processes belonging to the parent and must not be driven + or shut down from here. + """ + _POOL_EXECUTOR_CACHE.clear() + + +atexit.register(_clear_pool_executor_cache) +os.register_at_fork(after_in_child=_forget_pool_executor_cache) + + +@contextlib.contextmanager +def _pool_executor( + pool_executor_class: _EXECUTOR_TYPE, max_workers: int +) -> Generator[concurrent.futures.Executor]: + """Provide a pool executor of the requested type and size. + + Parameters + ---------- + pool_executor_class : `type` [ `concurrent.futures.Executor` ] + Type of executor pool to use. + max_workers : `int` + Number of workers the pool should use. + + Yields + ------ + executor : `concurrent.futures.Executor` + The executor to submit work to. + + Notes + ----- + A process pool outlives the block and is reused by later calls, so its + workers pay their startup cost once. A pool that has broken is discarded + so that the next caller is given a fresh one. A thread pool is created for + the block and shut down when it ends. + """ + if not issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): + with _make_pool_executor(pool_executor_class, max_workers) as transient: + yield transient + return + + key = (pool_executor_class, max_workers) + executor = _POOL_EXECUTOR_CACHE.get(key) + if executor is None: + executor = _make_pool_executor(pool_executor_class, max_workers) + _POOL_EXECUTOR_CACHE[key] = executor + try: + yield executor + except concurrent.futures.BrokenExecutor: + _discard_pool_executor(key) + raise + + +def _discard_pool_executor(key: tuple[_EXECUTOR_TYPE, int]) -> None: + """Drop a cached pool executor so that the next caller gets a fresh one. + + Parameters + ---------- + key : `tuple` [ `type`, `int` ] + Executor class and worker count identifying the cached pool. + """ + executor = _POOL_EXECUTOR_CACHE.pop(key, None) + if executor is not None: + executor.shutdown(wait=False) + + @dataclasses.dataclass(frozen=True) class ResourceInfo: """Information about this resource.""" @@ -1082,7 +1170,7 @@ def _mexists_pool( Mapping of original URI to boolean indicating existence. """ max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) - with _make_pool_executor(pool_executor_class, max_workers) as exists_executor: + with _pool_executor(pool_executor_class, max_workers) as exists_executor: future_exists = {exists_executor.submit(uri.exists): uri for uri in uris} results: dict[ResourcePath, bool] = {} @@ -1181,7 +1269,7 @@ def _mtransfer( whether the transfer succeeded for the target URI. """ max_workers = _get_num_workers() - with _make_pool_executor(pool_executor_class, max_workers) as transfer_executor: + with _pool_executor(pool_executor_class, max_workers) as transfer_executor: future_transfers = { transfer_executor.submit( to_uri.transfer_from, @@ -1326,7 +1414,7 @@ def _mremove_pool( max_workers = min(max_workers, len(chunks)) results: dict[ResourcePath, MBulkResult] = {} - with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: + with _pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = {remove_executor.submit(cls._remove_chunk, chunk): chunk for chunk in chunks} for future in concurrent.futures.as_completed(future_remove): try: diff --git a/python/lsst/resources/s3.py b/python/lsst/resources/s3.py index 5ca4a9c0..3fa3d393 100644 --- a/python/lsst/resources/s3.py +++ b/python/lsst/resources/s3.py @@ -41,7 +41,7 @@ ResourceInfo, ResourcePath, _get_executor_class, - _make_pool_executor, + _pool_executor, ) from .s3utils import ( _get_s3_connection_parameters, @@ -309,7 +309,7 @@ def _mremove_with_pool( # No need to make more workers than we have chunks. max_workers = num_workers if num_workers is not None else min(len(chunks), _get_num_workers()) results: dict[ResourcePath, MBulkResult] = {} - with _make_pool_executor(pool_executor_class, max_workers) as remove_executor: + with _pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = { remove_executor.submit(cls._delete_objects_wrapper, chunk): i for i, chunk in enumerate(chunks) diff --git a/tests/test_utils.py b/tests/test_utils.py index f2c08d0b..00532ea1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -17,7 +17,11 @@ from typing import Any from lsst.resources import ResourcePath -from lsst.resources._resourcePath import _make_pool_executor +from lsst.resources._resourcePath import ( + _clear_pool_executor_cache, + _make_pool_executor, + _pool_executor, +) from lsst.resources.file import FileResourcePath from lsst.resources.s3 import S3ResourcePath from lsst.resources.utils import ( @@ -152,5 +156,63 @@ def test_explicit_request_overrides_the_scheme_cap(self) -> None: self.assertEqual(_get_num_workers(S3ResourcePath._max_workers), 99) +class PoolReuseTestCase(unittest.TestCase): + """Tests for reuse of process pools across calls.""" + + def tearDown(self) -> None: + _clear_pool_executor_cache() + + def test_process_pools_are_reused(self) -> None: + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as first: + pass + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as second: + pass + self.assertIs(first, second) + # Still usable after both blocks have exited. + self.assertEqual(list(second.map(int, ["1", "2"])), [1, 2]) + + def test_different_sizes_get_different_pools(self) -> None: + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as small: + pass + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 3) as large: + pass + self.assertIsNot(small, large) + + def test_thread_pools_are_not_reused(self) -> None: + with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as first: + pass + with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as second: + pass + self.assertIsNot(first, second) + # A thread pool is cheap, so it is shut down when the block ends. + with self.assertRaises(RuntimeError): + first.submit(int, "1") + + def test_clearing_the_cache_shuts_pools_down(self) -> None: + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as executor: + pass + _clear_pool_executor_cache() + with self.assertRaises(RuntimeError): + executor.submit(int, "1") + # The next request builds a fresh pool. + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: + self.assertIsNot(replacement, executor) + + def test_a_broken_pool_is_replaced(self) -> None: + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as executor: + # Workers start lazily, so submit before there is anything to kill. + list(executor.map(int, ["1", "2"])) + # Kill the workers so the pool is unusable. + for process in list(executor._processes.values()): + process.terminate() + process.join() + with self.assertRaises(concurrent.futures.BrokenExecutor): + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as broken: + broken.submit(int, "1").result() + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: + self.assertIsNot(replacement, executor) + self.assertEqual(replacement.submit(int, "1").result(), 1) + + if __name__ == "__main__": unittest.main() From 6bbe89a77d318d6dec41dbf93911123018d2da09 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Mon, 14 Sep 2026 15:55:14 -0700 Subject: [PATCH 08/20] Send batches of URIs to each existence-check worker Existence checking now uses the same batching as removal, and the chunking helper is renamed to reflect that it serves both. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.perf.rst | 4 +- python/lsst/resources/_resourcePath.py | 49 +++++++++++++++++---- tests/test_file.py | 60 ++++++++++++++++++++++++-- 3 files changed, 99 insertions(+), 14 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 0ebe0266..889d2c35 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,4 +1,4 @@ -``ResourcePath.mremove()`` now sends a batch of URIs to each worker rather than submitting one task per URI. +``ResourcePath.mremove()`` and ``ResourcePath.mexists()`` now send a batch of URIs to each worker rather than submitting one task per URI. Process pools are now reused between bulk operations instead of being created and destroyed for each call. -Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that removes many artifacts. +Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that performs many bulk operations. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 9c6970b4..9d1bfbcd 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -1169,18 +1169,49 @@ def _mexists_pool( existence : `dict` of [`ResourcePath`, `bool`] Mapping of original URI to boolean indicating existence. """ + uri_list = list(uris) max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) - with _pool_executor(pool_executor_class, max_workers) as exists_executor: - future_exists = {exists_executor.submit(uri.exists): uri for uri in uris} + chunks = cls._chunk_uris(uri_list, max_workers) + if not chunks: + return {} + # No need for more workers than there are chunks to give them. + max_workers = min(max_workers, len(chunks)) - results: dict[ResourcePath, bool] = {} + results: dict[ResourcePath, bool] = {} + with _pool_executor(pool_executor_class, max_workers) as exists_executor: + future_exists = {exists_executor.submit(cls._exists_chunk, chunk): chunk for chunk in chunks} for future in concurrent.futures.as_completed(future_exists): - uri = future_exists[future] try: - exists = future.result() + results.update(future.result()) except Exception: - exists = False - results[uri] = exists + # The chunk failed as a whole, for example because a + # worker died. + for uri in future_exists[future]: + results[uri] = False + return results + + @classmethod + def _exists_chunk(cls, uris: tuple[ResourcePath, ...]) -> dict[ResourcePath, bool]: + """Check a batch of URIs for existence. + + Parameters + ---------- + uris : `tuple` [ `ResourcePath`, ... ] + The URIs to check. + + Returns + ------- + results : `dict` [ `ResourcePath`, `bool` ] + An entry for every URI in ``uris``. A URI that cannot be checked + is reported as absent, and does not prevent the URIs after it in + the batch from being checked. + """ + results: dict[ResourcePath, bool] = {} + for uri in uris: + try: + results[uri] = uri.exists() + except Exception: + results[uri] = False return results @classmethod @@ -1351,7 +1382,7 @@ def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResul return cls._mremove_pool(_get_executor_class(), uris) @staticmethod - def _chunk_for_removal(uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: + def _chunk_uris(uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: """Split URIs into batches sized for the given number of workers. Parameters @@ -1407,7 +1438,7 @@ def _mremove_pool( """Remove URIs using a futures pool.""" uri_list = list(uris) max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) - chunks = cls._chunk_for_removal(uri_list, max_workers) + chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} # No need for more workers than there are chunks to give them. diff --git a/tests/test_file.py b/tests/test_file.py index e30dabc2..d88ec431 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -311,20 +311,74 @@ def test_failure_does_not_abandon_the_rest_of_the_chunk(self) -> None: def test_chunk_sizes(self) -> None: uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] # Fewer URIs than chunk slots gives one URI per chunk. - chunks = FileResourcePath._chunk_for_removal(uris, 32) + chunks = FileResourcePath._chunk_uris(uris, 32) self.assertEqual(len(chunks), 5) self.assertTrue(all(len(c) == 1 for c in chunks)) # More URIs than chunk slots gives evenly sized chunks. - chunks = FileResourcePath._chunk_for_removal(uris, 1) + chunks = FileResourcePath._chunk_uris(uris, 1) self.assertEqual([len(c) for c in chunks], [2, 2, 1]) # An empty input yields no chunks at all. - self.assertEqual(FileResourcePath._chunk_for_removal([], 4), []) + self.assertEqual(FileResourcePath._chunk_uris([], 4), []) def test_empty_removal_is_a_no_op(self) -> None: self.assertEqual(ResourcePath.mremove([]), {}) + def test_exists_chunk_reports_each_uri(self) -> None: + present = [self.tmpdir.join(f"p{n}.txt") for n in range(3)] + for uri in present: + uri.write(b"") + absent = self.tmpdir.join("gone.txt") + + results = FileResourcePath._exists_chunk((*present, absent)) + + self.assertEqual(len(results), 4) + self.assertTrue(all(results[uri] for uri in present)) + self.assertFalse(results[absent]) + + def test_exists_chunk_treats_an_error_as_missing(self) -> None: + uris = [self.tmpdir.join(f"e{n}.txt") for n in range(3)] + for uri in uris: + uri.write(b"") + failing = uris[1].ospath + real_exists = FileResourcePath.exists + + def flaky(self: FileResourcePath) -> bool: + if self.ospath == failing: + raise PermissionError("cannot stat") + return real_exists(self) + + with unittest.mock.patch.object(FileResourcePath, "exists", flaky): + results = FileResourcePath._exists_chunk(tuple(uris)) + + # The failure is reported as absent and the rest are still checked. + self.assertEqual(results, {uris[0]: True, uris[1]: False, uris[2]: True}) + + @unittest.mock.patch.dict(os.environ, {}, clear=False) + @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + def test_mexists_scheme_cap_sizes_the_pool(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + recorded: list[int] = [] + + class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): + def __init__(self, max_workers: int, **kwargs: Any) -> None: + recorded.append(max_workers) + super().__init__(max_workers=max_workers, **kwargs) + + uris = [self.tmpdir.join(f"x{n}.txt") for n in range(20)] + for uri in uris: + uri.write(b"") + + results = FileResourcePath._mexists_pool(_RecordingExecutor, uris) + + self.assertEqual(recorded, [3]) + self.assertTrue(all(results.values())) + + def test_empty_existence_check_is_a_no_op(self) -> None: + self.assertEqual(ResourcePath.mexists([]), {}) + @unittest.mock.patch.dict(os.environ, {}, clear=False) @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) def test_scheme_cap_sizes_the_pool(self) -> None: From 1997c848731e617b9e8ec216276e12d697693d07 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 12:49:44 -0700 Subject: [PATCH 09/20] Reuse only one cached process pool Replace the cached pool when the executor class or worker count changes. Shut down the old pool so varying batch sizes do not accumulate workers. Add regression coverage for replacement and shutdown. Co-authored-by: Codex --- doc/changes/DM-56097.perf.rst | 2 +- python/lsst/resources/_resourcePath.py | 57 +++++++++++++++----------- tests/test_utils.py | 16 ++++++-- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 889d2c35..0537a52d 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,4 +1,4 @@ ``ResourcePath.mremove()`` and ``ResourcePath.mexists()`` now send a batch of URIs to each worker rather than submitting one task per URI. -Process pools are now reused between bulk operations instead of being created and destroyed for each call. +A single process pool is now reused between bulk operations instead of being created and destroyed for each call; changing the executor class or worker count replaces it. Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that performs many bulk operations. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 9d1bfbcd..0679caa9 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -147,24 +147,27 @@ def _make_pool_executor(pool_executor_class: _EXECUTOR_TYPE, max_workers: int) - return pool_executor_class(max_workers=max_workers) -# Process pools, keyed by executor class and worker count. Starting one costs -# a full interpreter startup per worker under the spawn start method, and the -# cost scales with how much the parent process has imported, so a pool is kept -# alive for reuse by later bulk operations. Thread pools are not cached; they -# cost almost nothing to create and holding one open would keep its threads -# alive for no benefit. -_POOL_EXECUTOR_CACHE: dict[tuple[_EXECUTOR_TYPE, int], concurrent.futures.Executor] = {} +# One process pool, identified by executor class and worker count. Starting one +# costs a full interpreter startup per worker under the spawn start method. +# That cost scales with how much the parent process has imported, so we keep +# the pool alive for reuse by later bulk operations. Thread pools are not +# cached; they cost almost nothing to create and holding one open would keep +# its threads alive for no benefit. +_POOL_EXECUTOR_CACHE: tuple[_EXECUTOR_TYPE, int, concurrent.futures.Executor] | None = None def _clear_pool_executor_cache() -> None: - """Shut down and forget every cached pool executor.""" - while _POOL_EXECUTOR_CACHE: - _, executor = _POOL_EXECUTOR_CACHE.popitem() + """Shut down and forget the cached pool executor.""" + global _POOL_EXECUTOR_CACHE + cached = _POOL_EXECUTOR_CACHE + _POOL_EXECUTOR_CACHE = None + if cached is not None: + _, _, executor = cached executor.shutdown(wait=True) def _forget_pool_executor_cache() -> None: - """Forget every cached pool executor without shutting it down. + """Forget the cached pool executor without shutting it down. Notes ----- @@ -172,7 +175,8 @@ def _forget_pool_executor_cache() -> None: refer to worker processes belonging to the parent and must not be driven or shut down from here. """ - _POOL_EXECUTOR_CACHE.clear() + global _POOL_EXECUTOR_CACHE + _POOL_EXECUTOR_CACHE = None atexit.register(_clear_pool_executor_cache) @@ -200,38 +204,43 @@ def _pool_executor( Notes ----- A process pool outlives the block and is reused by later calls, so its - workers pay their startup cost once. A pool that has broken is discarded + workers pay their startup cost once. A different executor class or worker + count replaces the cached pool. A pool that has broken is discarded so that the next caller is given a fresh one. A thread pool is created for the block and shut down when it ends. """ + global _POOL_EXECUTOR_CACHE if not issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): with _make_pool_executor(pool_executor_class, max_workers) as transient: yield transient return - key = (pool_executor_class, max_workers) - executor = _POOL_EXECUTOR_CACHE.get(key) - if executor is None: + cached = _POOL_EXECUTOR_CACHE + if cached is None or cached[:2] != (pool_executor_class, max_workers): + _clear_pool_executor_cache() executor = _make_pool_executor(pool_executor_class, max_workers) - _POOL_EXECUTOR_CACHE[key] = executor + _POOL_EXECUTOR_CACHE = (pool_executor_class, max_workers, executor) + else: + executor = cached[2] try: yield executor except concurrent.futures.BrokenExecutor: - _discard_pool_executor(key) + _discard_pool_executor(executor) raise -def _discard_pool_executor(key: tuple[_EXECUTOR_TYPE, int]) -> None: +def _discard_pool_executor(executor: concurrent.futures.Executor) -> None: """Drop a cached pool executor so that the next caller gets a fresh one. Parameters ---------- - key : `tuple` [ `type`, `int` ] - Executor class and worker count identifying the cached pool. + executor : `concurrent.futures.Executor` + Broken executor. Only clears the cache if it still holds this pool. """ - executor = _POOL_EXECUTOR_CACHE.pop(key, None) - if executor is not None: - executor.shutdown(wait=False) + global _POOL_EXECUTOR_CACHE + if _POOL_EXECUTOR_CACHE is not None and _POOL_EXECUTOR_CACHE[2] is executor: + _POOL_EXECUTOR_CACHE = None + executor.shutdown(wait=False) @dataclasses.dataclass(frozen=True) diff --git a/tests/test_utils.py b/tests/test_utils.py index 00532ea1..8111164e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -171,12 +171,21 @@ def test_process_pools_are_reused(self) -> None: # Still usable after both blocks have exited. self.assertEqual(list(second.map(int, ["1", "2"])), [1, 2]) - def test_different_sizes_get_different_pools(self) -> None: + def test_different_sizes_replace_the_cached_pool(self) -> None: with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as small: - pass + self.assertEqual(small.submit(int, "1").result(), 1) with _pool_executor(concurrent.futures.ProcessPoolExecutor, 3) as large: - pass + self.assertEqual(large.submit(int, "2").result(), 2) self.assertIsNot(small, large) + with self.assertRaises(RuntimeError): + small.submit(int, "1") + # Returning to an earlier size creates a new pool, and also shuts + # down the larger one instead of leaving its workers alive. + with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: + self.assertIsNot(replacement, small) + self.assertEqual(replacement.submit(int, "3").result(), 3) + with self.assertRaises(RuntimeError): + large.submit(int, "1") def test_thread_pools_are_not_reused(self) -> None: with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as first: @@ -213,6 +222,5 @@ def test_a_broken_pool_is_replaced(self) -> None: self.assertIsNot(replacement, executor) self.assertEqual(replacement.submit(int, "1").result(), 1) - if __name__ == "__main__": unittest.main() From 0314ccbfa9a80823fbd3f2c76bf684af1ab6620b Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 12:49:53 -0700 Subject: [PATCH 10/20] Discard broken process pools during bulk operations Evict broken pools when bulk result handlers catch worker failures. Preserve per-resource failure reports while allowing the next caller to create a healthy pool. Cover existence, removal, transfer, and S3 removal. Co-authored-by: Codex --- doc/changes/DM-56097.bugfix.rst | 1 + python/lsst/resources/_resourcePath.py | 8 ++++- python/lsst/resources/s3.py | 3 ++ tests/test_utils.py | 41 ++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/doc/changes/DM-56097.bugfix.rst b/doc/changes/DM-56097.bugfix.rst index 4794977a..d51ee018 100644 --- a/doc/changes/DM-56097.bugfix.rst +++ b/doc/changes/DM-56097.bugfix.rst @@ -1,3 +1,4 @@ Fixed the number of workers used by bulk operations when ``$LSST_RESOURCES_EXECUTOR`` is set to ``process``. The worker count for a subprocess is now set by a pool initializer rather than through the environment, so it no longer depends on whether the parent process had already calculated its own worker count, and it behaves the same under the ``fork`` and ``spawn`` start methods. ``ResourcePath.mexists()`` now honors an explicit ``num_workers`` argument when a process pool is in use. +Bulk operations discard broken process pools so subsequent calls can create a fresh pool. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 0679caa9..3b72226e 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -1192,9 +1192,11 @@ def _mexists_pool( for future in concurrent.futures.as_completed(future_exists): try: results.update(future.result()) - except Exception: + except Exception as e: # The chunk failed as a whole, for example because a # worker died. + if isinstance(e, concurrent.futures.BrokenExecutor): + _discard_pool_executor(exists_executor) for uri in future_exists[future]: results[uri] = False return results @@ -1328,6 +1330,8 @@ def _mtransfer( try: future.result() except Exception as e: + if isinstance(e, concurrent.futures.BrokenExecutor): + _discard_pool_executor(transfer_executor) transferred = MBulkResult(False, e) failed = True else: @@ -1462,6 +1466,8 @@ def _mremove_pool( except Exception as e: # The chunk failed as a whole, for example because a # worker died. + if isinstance(e, concurrent.futures.BrokenExecutor): + _discard_pool_executor(remove_executor) for uri in future_remove[future]: results[uri] = MBulkResult(False, e) return results diff --git a/python/lsst/resources/s3.py b/python/lsst/resources/s3.py index 3fa3d393..d369895d 100644 --- a/python/lsst/resources/s3.py +++ b/python/lsst/resources/s3.py @@ -40,6 +40,7 @@ MBulkResult, ResourceInfo, ResourcePath, + _discard_pool_executor, _get_executor_class, _pool_executor, ) @@ -319,6 +320,8 @@ def _mremove_with_pool( results.update(future.result()) except Exception as e: # The chunk utterly failed. + if isinstance(e, concurrent.futures.BrokenExecutor): + _discard_pool_executor(remove_executor) chunk = chunks[future_remove[future]] for uri in chunk: results[uri] = MBulkResult(False, e) diff --git a/tests/test_utils.py b/tests/test_utils.py index 8111164e..573809d9 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -14,6 +14,7 @@ import os import unittest import unittest.mock +from concurrent.futures.process import BrokenProcessPool from typing import Any from lsst.resources import ResourcePath @@ -222,5 +223,45 @@ def test_a_broken_pool_is_replaced(self) -> None: self.assertIsNot(replacement, executor) self.assertEqual(replacement.submit(int, "1").result(), 1) + def test_bulk_operations_discard_broken_pools(self) -> None: + uris = [ResourcePath(__file__), ResourcePath(__file__).updatedFile("missing.txt")] + executor_class = concurrent.futures.ProcessPoolExecutor + operations = { + "mexists": lambda: FileResourcePath._mexists_pool(executor_class, uris, num_workers=2), + "mremove": lambda: FileResourcePath._mremove_pool(executor_class, uris, num_workers=2), + "mtransfer": lambda: ResourcePath._mtransfer( + executor_class, "copy", [(uris[0], uris[1])], do_raise=False + ), + "s3_mremove": lambda: S3ResourcePath._mremove_with_pool( + executor_class, [(uris[0],), (uris[1],)], num_workers=2 + ), + } + for name, operation in operations.items(): + with self.subTest(operation=name): + with _pool_executor(executor_class, 2) as broken: + pass + + def fail_submission(*args: Any, **kwargs: Any) -> concurrent.futures.Future: + future = concurrent.futures.Future() + future.set_exception(BrokenProcessPool("worker died")) + return future + + # Fail the futures, not submit(), to exercise the exceptions + # caught inside each bulk operation's result loop. + with ( + unittest.mock.patch.object(broken, "submit", side_effect=fail_submission), + unittest.mock.patch("lsst.resources._resourcePath._get_num_workers", return_value=2), + ): + results = operation() + if name == "mexists": + self.assertTrue(all(value is False for value in results.values())) + else: + self.assertTrue(all(not value.success for value in results.values())) + with _pool_executor(executor_class, 2) as replacement: + self.assertIsNot(replacement, broken) + self.assertEqual(replacement.submit(int, "1").result(), 1) + + + if __name__ == "__main__": unittest.main() From 854426a4063cef87bbb895ad61b9fe8d59b5112c Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 12:50:05 -0700 Subject: [PATCH 11/20] Guard fork-hook registration on Windows Register the cache cleanup hook only when os.register_at_fork exists. Test module loading without the hook and only exercise supported process start methods in worker tests. Co-authored-by: Codex --- doc/changes/DM-56097.bugfix.rst | 1 + python/lsst/resources/_resourcePath.py | 3 ++- tests/test_utils.py | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/doc/changes/DM-56097.bugfix.rst b/doc/changes/DM-56097.bugfix.rst index d51ee018..7f181e5f 100644 --- a/doc/changes/DM-56097.bugfix.rst +++ b/doc/changes/DM-56097.bugfix.rst @@ -2,3 +2,4 @@ Fixed the number of workers used by bulk operations when ``$LSST_RESOURCES_EXECU The worker count for a subprocess is now set by a pool initializer rather than through the environment, so it no longer depends on whether the parent process had already calculated its own worker count, and it behaves the same under the ``fork`` and ``spawn`` start methods. ``ResourcePath.mexists()`` now honors an explicit ``num_workers`` argument when a process pool is in use. Bulk operations discard broken process pools so subsequent calls can create a fresh pool. +Fork-hook registration is conditional so the package can still be imported on Windows. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 3b72226e..7764f301 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -180,7 +180,8 @@ def _forget_pool_executor_cache() -> None: atexit.register(_clear_pool_executor_cache) -os.register_at_fork(after_in_child=_forget_pool_executor_cache) +if hasattr(os, "register_at_fork"): + os.register_at_fork(after_in_child=_forget_pool_executor_cache) @contextlib.contextmanager diff --git a/tests/test_utils.py b/tests/test_utils.py index 573809d9..c7e1fcfd 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -12,6 +12,8 @@ import concurrent.futures import multiprocessing import os +import subprocess +import sys import unittest import unittest.mock from concurrent.futures.process import BrokenProcessPool @@ -87,6 +89,8 @@ def test_process_worker_reports_one_worker(self) -> None: # The fork start method is the one where a child inherits the parent's # memoized state, so both methods must be checked. for method in ("fork", "spawn"): + if method not in multiprocessing.get_all_start_methods(): + continue with self.subTest(start_method=method): parent_before = _get_num_workers() context = multiprocessing.get_context(method) @@ -261,6 +265,21 @@ def fail_submission(*args: Any, **kwargs: Any) -> concurrent.futures.Future: self.assertIsNot(replacement, broken) self.assertEqual(replacement.submit(int, "1").result(), 1) + def test_import_without_fork_support(self) -> None: + # Load dependencies before hiding the hook: POSIX versions of some + # stdlib modules (such as random) assume the hook is available. + subprocess.run( + [ + sys.executable, + "-c", + "import os, importlib; " + "import lsst.resources._resourcePath as resource_path; " + "hasattr(os, 'register_at_fork') and delattr(os, 'register_at_fork'); " + "importlib.reload(resource_path)", + ], + check=True, + capture_output=True, + ) if __name__ == "__main__": From 7899b872739545983f6466852396520d2650b601 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 13:22:48 -0700 Subject: [PATCH 12/20] Size worker pools from the scheme rather than the batch Three call sites reduced the worker count to the number of chunks a particular batch produced. With a single cached process pool that makes the pool's size a function of the batch size, so a small call replaces the pool built for a large one and the next large call has to build it again. Under the spawn start method each rebuild costs a full interpreter startup per worker. Asking for more workers than there is work for them costs nothing: ProcessPoolExecutor starts workers on demand for every start method except fork, and under fork a worker is cheap. Generated with AI Co-Authored-By: SLAC AI --- doc/changes/DM-56097.perf.rst | 1 + python/lsst/resources/_resourcePath.py | 15 ++++++----- python/lsst/resources/s3.py | 3 +-- tests/test_utils.py | 37 ++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 0537a52d..b03ebd24 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,4 +1,5 @@ ``ResourcePath.mremove()`` and ``ResourcePath.mexists()`` now send a batch of URIs to each worker rather than submitting one task per URI. A single process pool is now reused between bulk operations instead of being created and destroyed for each call; changing the executor class or worker count replaces it. Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that performs many bulk operations. +The number of workers no longer depends on how many URIs a call is given, so a sequence of calls with differing batch sizes reuses one pool rather than replacing it each time. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 7764f301..e96c4763 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -150,9 +150,14 @@ def _make_pool_executor(pool_executor_class: _EXECUTOR_TYPE, max_workers: int) - # One process pool, identified by executor class and worker count. Starting one # costs a full interpreter startup per worker under the spawn start method. # That cost scales with how much the parent process has imported, so we keep -# the pool alive for reuse by later bulk operations. Thread pools are not -# cached; they cost almost nothing to create and holding one open would keep -# its threads alive for no benefit. +# the pool alive for reuse by later bulk operations. Callers must therefore +# size a pool from the scheme and the configuration alone, and never from the +# number of URIs in hand, since a size that varies from call to call would +# replace the cached pool on every call. Asking for more workers than there is +# work to give them is harmless: only the fork start method starts them all up +# front, and there a worker is cheap. Thread pools are not cached; they cost +# almost nothing to create and holding one open would keep its threads alive +# for no benefit. _POOL_EXECUTOR_CACHE: tuple[_EXECUTOR_TYPE, int, concurrent.futures.Executor] | None = None @@ -1184,8 +1189,6 @@ def _mexists_pool( chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} - # No need for more workers than there are chunks to give them. - max_workers = min(max_workers, len(chunks)) results: dict[ResourcePath, bool] = {} with _pool_executor(pool_executor_class, max_workers) as exists_executor: @@ -1455,8 +1458,6 @@ def _mremove_pool( chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} - # No need for more workers than there are chunks to give them. - max_workers = min(max_workers, len(chunks)) results: dict[ResourcePath, MBulkResult] = {} with _pool_executor(pool_executor_class, max_workers) as remove_executor: diff --git a/python/lsst/resources/s3.py b/python/lsst/resources/s3.py index d369895d..d32952c3 100644 --- a/python/lsst/resources/s3.py +++ b/python/lsst/resources/s3.py @@ -307,8 +307,7 @@ def _mremove_with_pool( num_workers: int | None = None, ) -> dict[ResourcePath, MBulkResult]: # Different name because different API to base class. - # No need to make more workers than we have chunks. - max_workers = num_workers if num_workers is not None else min(len(chunks), _get_num_workers()) + max_workers = num_workers if num_workers is not None else _get_num_workers() results: dict[ResourcePath, MBulkResult] = {} with _pool_executor(pool_executor_class, max_workers) as remove_executor: future_remove = { diff --git a/tests/test_utils.py b/tests/test_utils.py index c7e1fcfd..f9e598cd 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -19,6 +19,7 @@ from concurrent.futures.process import BrokenProcessPool from typing import Any +import lsst.resources._resourcePath as resource_path from lsst.resources import ResourcePath from lsst.resources._resourcePath import ( _clear_pool_executor_cache, @@ -192,6 +193,42 @@ def test_different_sizes_replace_the_cached_pool(self) -> None: with self.assertRaises(RuntimeError): large.submit(int, "1") + @unittest.mock.patch.dict(os.environ, {}, clear=False) + @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + def test_batch_size_does_not_replace_the_cached_pool(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + _clear_pool_executor_cache() + one = [ResourcePath(__file__)] + many = [ResourcePath(__file__).updatedFile(f"missing{n}.txt") for n in range(64)] + + # A batch small enough to occupy a single worker must still be given a + # pool sized for the scheme, or the next larger batch would replace it. + for uris in (one, many, one): + FileResourcePath._mexists_pool(concurrent.futures.ProcessPoolExecutor, uris) + cached = resource_path._POOL_EXECUTOR_CACHE + assert cached is not None + self.assertEqual(cached[1], 3) + if uris is one: + first = cached[2] + self.assertIs(resource_path._POOL_EXECUTOR_CACHE[2], first) + + @unittest.mock.patch.dict(os.environ, {}, clear=False) + def test_s3_batch_size_does_not_size_the_pool(self) -> None: + os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) + _clear_worker_caches() + recorded: list[int] = [] + + class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): + def __init__(self, max_workers: int, **kwargs: Any) -> None: + recorded.append(max_workers) + super().__init__(max_workers=max_workers, **kwargs) + + uri = ResourcePath("s3://bucket/object.txt") + with unittest.mock.patch.object(S3ResourcePath, "_delete_objects_wrapper", return_value={}): + S3ResourcePath._mremove_with_pool(_RecordingExecutor, [(uri,)]) + self.assertEqual(recorded, [_get_num_workers()]) + def test_thread_pools_are_not_reused(self) -> None: with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as first: pass From f22781b68b465d54a147bb6175eeb831083ea73f Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 13:33:35 -0700 Subject: [PATCH 13/20] Do small batches in the calling thread Splitting a batch into one URI per chunk meant that checking three files for existence occupied three workers, and under a process executor could build a pool of subprocesses to do it. Handing the work over costs more than the work itself at that size. Chunks now have a per-scheme floor on their size, and a batch that produces a single chunk is handled directly by the caller with no executor involved. The floor is a scheme's property rather than a global one because it expresses the cost of one operation: a missing-file check on a local filesystem takes around 100 microseconds, so a batch has to reach about a hundred URIs before spreading it wins, whereas a scheme whose every operation is a network round trip is worth overlapping for two. This generalizes the single-chunk shortcut that S3 bulk removal already had. Measured on wekafs with missing files, best of five, milliseconds: threads process N before after before after 1 0.37 0.08 20.38 0.07 3 0.54 0.22 20.60 0.25 30 3.14 2.12 23.75 2.28 300 9.56 10.03 31.48 36.31 1000 22.79 21.34 48.10 55.65 10000 270.81 213.09 266.96 259.08 Generated with AI Co-Authored-By: SLAC AI --- doc/changes/DM-56097.perf.rst | 2 ++ python/lsst/resources/_resourcePath.py | 34 +++++++++++++++++++++--- python/lsst/resources/file.py | 5 ++++ tests/test_file.py | 36 ++++++++++++++++++++++++++ tests/test_utils.py | 10 ++++--- 5 files changed, 79 insertions(+), 8 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index b03ebd24..76ffb3bb 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -3,3 +3,5 @@ A single process pool is now reused between bulk operations instead of being cre Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that performs many bulk operations. The number of workers no longer depends on how many URIs a call is given, so a sequence of calls with differing batch sizes reuses one pool rather than replacing it each time. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. +A batch of URIs too small to be worth spreading over workers is now checked or removed in the calling thread, since handing a couple of local file checks to a thread or subprocess costs more than doing them. +The size below which this applies is set per scheme, because a scheme whose every operation is a network round trip benefits from overlapping even two URIs. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index e96c4763..ea1149cc 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -347,6 +347,16 @@ class ResourcePath: # numpydoc ignore=PR02 sized to match it; schemes with no pool can raise it. """ + _min_chunk_size: int = 1 + """Smallest batch of URIs worth giving to a worker of its own. + + A batch no larger than this is handled in the calling thread instead of + being sent to a pool, since handing work to another thread or process + costs more than doing it. The default suits a scheme where every + operation is a network round trip and so is worth overlapping even for a + couple of URIs. A scheme whose operations are cheap should raise it. + """ + # This is not an ABC with abstract methods because the __new__ being # a factory confuses mypy such that it assumes that every constructor # returns a ResourcePath and then determines that all the abstract methods @@ -1189,6 +1199,10 @@ def _mexists_pool( chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} + if len(chunks) == 1: + # Not enough work to be worth handing to another thread or + # process. + return cls._exists_chunk(chunks[0]) results: dict[ResourcePath, bool] = {} with _pool_executor(pool_executor_class, max_workers) as exists_executor: @@ -1398,8 +1412,8 @@ def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResul """Remove multiple URIs using futures.""" return cls._mremove_pool(_get_executor_class(), uris) - @staticmethod - def _chunk_uris(uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: + @classmethod + def _chunk_uris(cls, uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: """Split URIs into batches sized for the given number of workers. Parameters @@ -1412,11 +1426,19 @@ def _chunk_uris(uris: list[ResourcePath], max_workers: int) -> list[tuple[Resour Returns ------- chunks : `list` [ `tuple` [ `ResourcePath`, ... ] ] - The batches. Empty if ``uris`` is empty. + The batches. Empty if ``uris`` is empty. A single batch means the + work is not worth spreading, and callers run it directly. + + Notes + ----- + Several batches per worker let a worker that draws quick URIs move on + to more of them, but no batch is smaller than ``_min_chunk_size``, + below which the cost of handing the batch over exceeds the cost of + the work in it. """ if not uris: return [] - chunk_size = max(1, math.ceil(len(uris) / (max_workers * CHUNKS_PER_WORKER))) + chunk_size = max(cls._min_chunk_size, math.ceil(len(uris) / (max_workers * CHUNKS_PER_WORKER))) return list(chunk_iterable(uris, chunk_size=chunk_size)) @classmethod @@ -1458,6 +1480,10 @@ def _mremove_pool( chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} + if len(chunks) == 1: + # Not enough work to be worth handing to another thread or + # process. + return cls._remove_chunk(chunks[0]) results: dict[ResourcePath, MBulkResult] = {} with _pool_executor(pool_executor_class, max_workers) as remove_executor: diff --git a/python/lsst/resources/file.py b/python/lsst/resources/file.py index 5bef1b73..93d41b42 100644 --- a/python/lsst/resources/file.py +++ b/python/lsst/resources/file.py @@ -84,6 +84,11 @@ class FileResourcePath(ResourcePath): # By definition refers to a local file isLocal = True + # A missing-file check on a local or cluster filesystem takes on the order + # of 100 microseconds, so a batch has to be around this large before + # spreading it over workers beats a plain loop. + _min_chunk_size = 100 + @property def ospath(self) -> str: """Path component of the URI localized to current OS. diff --git a/tests/test_file.py b/tests/test_file.py index d88ec431..219c5f37 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -19,6 +19,7 @@ import urllib.parse from typing import Any +import lsst.resources._resourcePath as resource_path from lsst.resources import ResourceInfo, ResourcePath, ResourcePathExpression from lsst.resources.file import FileResourcePath from lsst.resources.tests import GenericReadWriteTestCase, GenericTestCase @@ -308,6 +309,7 @@ def test_failure_does_not_abandon_the_rest_of_the_chunk(self) -> None: self.assertTrue(results[uri].success, f"{uri} should have been removed") self.assertFalse(uri.exists()) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_chunk_sizes(self) -> None: uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] # Fewer URIs than chunk slots gives one URI per chunk. @@ -322,6 +324,17 @@ def test_chunk_sizes(self) -> None: # An empty input yields no chunks at all. self.assertEqual(FileResourcePath._chunk_uris([], 4), []) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 4) + def test_chunk_size_floor(self) -> None: + uris = [self.tmpdir.join(f"f{n}.txt") for n in range(10)] + + # A batch that would otherwise be spread thinly is kept in one piece, + # which is the signal to the caller to handle it without a pool. + self.assertEqual([len(c) for c in FileResourcePath._chunk_uris(uris[:4], 32)], [4]) + + # The floor never makes chunks larger than the worker count calls for. + self.assertEqual([len(c) for c in FileResourcePath._chunk_uris(uris, 1)], [4, 4, 2]) + def test_empty_removal_is_a_no_op(self) -> None: self.assertEqual(ResourcePath.mremove([]), {}) @@ -357,6 +370,7 @@ def flaky(self: FileResourcePath) -> bool: @unittest.mock.patch.dict(os.environ, {}, clear=False) @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_mexists_scheme_cap_sizes_the_pool(self) -> None: os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) _clear_worker_caches() @@ -379,8 +393,30 @@ def __init__(self, max_workers: int, **kwargs: Any) -> None: def test_empty_existence_check_is_a_no_op(self) -> None: self.assertEqual(ResourcePath.mexists([]), {}) + def test_small_batches_avoid_the_pool(self) -> None: + present = [self.tmpdir.join(f"s{n}.txt") for n in range(3)] + for uri in present: + uri.write(b"") + absent = self.tmpdir.join("nothere.txt") + + def refuse(*args: Any, **kwargs: Any) -> None: + raise AssertionError("a batch this small must not be given to a pool") + + with unittest.mock.patch.object(resource_path, "_pool_executor", refuse): + existence = FileResourcePath._mexists_pool( + concurrent.futures.ThreadPoolExecutor, [*present, absent] + ) + removals = FileResourcePath._mremove_pool( + concurrent.futures.ThreadPoolExecutor, [*present, absent] + ) + + self.assertEqual(existence, {**dict.fromkeys(present, True), absent: False}) + self.assertTrue(all(removals[uri].success for uri in present)) + self.assertFalse(removals[absent].success) + @unittest.mock.patch.dict(os.environ, {}, clear=False) @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_scheme_cap_sizes_the_pool(self) -> None: os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) _clear_worker_caches() diff --git a/tests/test_utils.py b/tests/test_utils.py index f9e598cd..fd472f86 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -195,21 +195,22 @@ def test_different_sizes_replace_the_cached_pool(self) -> None: @unittest.mock.patch.dict(os.environ, {}, clear=False) @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_batch_size_does_not_replace_the_cached_pool(self) -> None: os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) _clear_worker_caches() _clear_pool_executor_cache() - one = [ResourcePath(__file__)] + few = [ResourcePath(__file__).updatedFile(f"missing{n}.txt") for n in range(2)] many = [ResourcePath(__file__).updatedFile(f"missing{n}.txt") for n in range(64)] - # A batch small enough to occupy a single worker must still be given a + # A batch that occupies only a couple of workers must still be given a # pool sized for the scheme, or the next larger batch would replace it. - for uris in (one, many, one): + for uris in (few, many, few): FileResourcePath._mexists_pool(concurrent.futures.ProcessPoolExecutor, uris) cached = resource_path._POOL_EXECUTOR_CACHE assert cached is not None self.assertEqual(cached[1], 3) - if uris is one: + if uris is few: first = cached[2] self.assertIs(resource_path._POOL_EXECUTOR_CACHE[2], first) @@ -264,6 +265,7 @@ def test_a_broken_pool_is_replaced(self) -> None: self.assertIsNot(replacement, executor) self.assertEqual(replacement.submit(int, "1").result(), 1) + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_bulk_operations_discard_broken_pools(self) -> None: uris = [ResourcePath(__file__), ResourcePath(__file__).updatedFile("missing.txt")] executor_class = concurrent.futures.ProcessPoolExecutor From cc3ce2f6d1b9b44b2d98ed46298b59a51589abd0 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 13:50:07 -0700 Subject: [PATCH 14/20] Clear checksum configuration in the S3 test fixture A site that sets AWS_REQUEST_CHECKSUM_CALCULATION to WHEN_REQUIRED stops boto3 computing a checksum on upload, so the object metadata carries no CRC32 and test_get_info fails on a developer machine while passing in CI. The fixture already removes the site's credentials and endpoint so that tests cannot reach real infrastructure; these two variables belong with them, because they change what the client does rather than where it points. Generated with AI Co-Authored-By: SLAC AI --- doc/changes/DM-56097.bugfix.rst | 1 + python/lsst/resources/s3utils.py | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/doc/changes/DM-56097.bugfix.rst b/doc/changes/DM-56097.bugfix.rst index 7f181e5f..38e08801 100644 --- a/doc/changes/DM-56097.bugfix.rst +++ b/doc/changes/DM-56097.bugfix.rst @@ -3,3 +3,4 @@ The worker count for a subprocess is now set by a pool initializer rather than t ``ResourcePath.mexists()`` now honors an explicit ``num_workers`` argument when a process pool is in use. Bulk operations discard broken process pools so subsequent calls can create a fresh pool. Fork-hook registration is conditional so the package can still be imported on Windows. +``clean_test_environment_for_s3()`` now also clears ``$AWS_REQUEST_CHECKSUM_CALCULATION`` and ``$AWS_RESPONSE_CHECKSUM_VALIDATION``, so that a site which disables checksums does not change the object metadata seen by tests. diff --git a/python/lsst/resources/s3utils.py b/python/lsst/resources/s3utils.py index 18aaab26..4f81aaa6 100644 --- a/python/lsst/resources/s3utils.py +++ b/python/lsst/resources/s3utils.py @@ -125,7 +125,8 @@ class _TooManyRequestsError(Exception): @contextmanager def clean_test_environment_for_s3() -> Generator[None]: """Reset S3 environment to ensure that unit tests with a mock S3 can't - accidentally reference real infrastructure. + accidentally reference real infrastructure, and that site configuration + cannot change how the client behaves. """ with patch.dict( os.environ, @@ -142,6 +143,10 @@ def clean_test_environment_for_s3() -> Generator[None]: "AWS_PROFILE", "AWS_SHARED_CREDENTIALS_FILE", "AWS_CONFIG_FILE", + # A site that turns checksums off changes what an object's + # metadata contains, so let the library defaults apply. + "AWS_REQUEST_CHECKSUM_CALCULATION", + "AWS_RESPONSE_CHECKSUM_VALIDATION", ): patched_environ.pop(var, None) # Clear the cached boto3 S3 client instances. From 57ee9ff3b2b677b5ac31caaed4488d467f9f9ba0 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 12:40:06 -0700 Subject: [PATCH 15/20] Use threads for all bulk operations Process pools are no faster than threads for remote URIs, and they silently discarded the undo actions that mtransfer registers on a caller's transaction: those were recorded in the worker process and never reached the parent, so a rollback would have left the transferred files in place. Removing them also drops the pool cache, the fork hooks and the worker marking that only process pools needed. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.bugfix.rst | 5 - doc/changes/DM-56097.perf.rst | 5 +- doc/changes/DM-56097.removal.rst | 3 + python/lsst/resources/_resourcePath.py | 316 +++---------------------- python/lsst/resources/s3.py | 26 +- python/lsst/resources/utils.py | 29 +-- tests/test_file.py | 100 +------- tests/test_s3.py | 27 --- tests/test_utils.py | 237 ------------------- 9 files changed, 41 insertions(+), 707 deletions(-) create mode 100644 doc/changes/DM-56097.removal.rst diff --git a/doc/changes/DM-56097.bugfix.rst b/doc/changes/DM-56097.bugfix.rst index 38e08801..51ae4e59 100644 --- a/doc/changes/DM-56097.bugfix.rst +++ b/doc/changes/DM-56097.bugfix.rst @@ -1,6 +1 @@ -Fixed the number of workers used by bulk operations when ``$LSST_RESOURCES_EXECUTOR`` is set to ``process``. -The worker count for a subprocess is now set by a pool initializer rather than through the environment, so it no longer depends on whether the parent process had already calculated its own worker count, and it behaves the same under the ``fork`` and ``spawn`` start methods. -``ResourcePath.mexists()`` now honors an explicit ``num_workers`` argument when a process pool is in use. -Bulk operations discard broken process pools so subsequent calls can create a fresh pool. -Fork-hook registration is conditional so the package can still be imported on Windows. ``clean_test_environment_for_s3()`` now also clears ``$AWS_REQUEST_CHECKSUM_CALCULATION`` and ``$AWS_RESPONSE_CHECKSUM_VALIDATION``, so that a site which disables checksums does not change the object metadata seen by tests. diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 76ffb3bb..5fd22f11 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,7 +1,4 @@ ``ResourcePath.mremove()`` and ``ResourcePath.mexists()`` now send a batch of URIs to each worker rather than submitting one task per URI. -A single process pool is now reused between bulk operations instead of being created and destroyed for each call; changing the executor class or worker count replaces it. -Under the ``spawn`` start method every worker pays a full interpreter startup, and that cost grows with how much the calling process has imported, so reuse matters most to a long-running process that performs many bulk operations. -The number of workers no longer depends on how many URIs a call is given, so a sequence of calls with differing batch sizes reuses one pool rather than replacing it each time. The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. -A batch of URIs too small to be worth spreading over workers is now checked or removed in the calling thread, since handing a couple of local file checks to a thread or subprocess costs more than doing them. +A batch of URIs too small to be worth spreading over workers is now checked or removed in the calling thread, since handing a couple of local file checks to a thread costs more than doing them. The size below which this applies is set per scheme, because a scheme whose every operation is a network round trip benefits from overlapping even two URIs. diff --git a/doc/changes/DM-56097.removal.rst b/doc/changes/DM-56097.removal.rst new file mode 100644 index 00000000..6ea5037f --- /dev/null +++ b/doc/changes/DM-56097.removal.rst @@ -0,0 +1,3 @@ +Removed support for the ``$LSST_RESOURCES_EXECUTOR`` environment variable. +Bulk operations now always use a thread pool. +Process pools were measured to be no faster for remote URIs and they silently discarded the undo actions registered by ``ResourcePath.mtransfer()`` on a caller's transaction, since those were recorded in the worker process and never reached the parent. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index ea1149cc..e8952fec 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -13,7 +13,6 @@ __all__ = ("ResourceInfo", "ResourcePath", "ResourcePathExpression") -import atexit import concurrent.futures import contextlib import copy @@ -31,7 +30,7 @@ from collections import defaultdict from pathlib import Path, PurePath, PurePosixPath from random import Random -from typing import TYPE_CHECKING, TypeAlias +from typing import TYPE_CHECKING try: import fsspec @@ -49,7 +48,7 @@ from lsst.utils.iteration import chunk_iterable from ._resourceHandles._baseResourceHandle import ResourceHandleProtocol -from .utils import MAX_WORKERS, _get_num_workers, _init_pool_worker, get_tempdir +from .utils import MAX_WORKERS, _get_num_workers, get_tempdir if TYPE_CHECKING: from .utils import TransactionProtocol @@ -75,180 +74,6 @@ class MBulkResult(NamedTuple): exception: Exception | None -_EXECUTOR_TYPE: TypeAlias = type[ - concurrent.futures.ThreadPoolExecutor | concurrent.futures.ProcessPoolExecutor -] - -# Cache value for executor class so as not to issue warning multiple -# times but still allow tests to override the value. -_POOL_EXECUTOR_CLASS: _EXECUTOR_TYPE | None = None - - -def _get_executor_class() -> _EXECUTOR_TYPE: - """Return the executor class used for parallelized execution. - - Returns - ------- - cls : `concurrent.futures.Executor` - The ``Executor`` class. Default is - `concurrent.futures.ThreadPoolExecutor`. Can be set explicitly by - setting the ``$LSST_RESOURCES_EXECUTOR`` environment variable to - "thread" or "process". Returns "thread" pool if the value of the - variable is not recognized. - """ - global _POOL_EXECUTOR_CLASS - - if _POOL_EXECUTOR_CLASS is not None: - return _POOL_EXECUTOR_CLASS - - pool_executor_classes = { - "threads": concurrent.futures.ThreadPoolExecutor, - "process": concurrent.futures.ProcessPoolExecutor, - } - default_executor = "threads" - external = os.getenv("LSST_RESOURCES_EXECUTOR", default_executor) - if not external: - external = default_executor - if external not in pool_executor_classes: - log.warning( - "Unrecognized value of '%s' for LSST_RESOURCES_EXECUTOR env var. Using '%s'", - external, - default_executor, - ) - external = default_executor - _POOL_EXECUTOR_CLASS = pool_executor_classes[external] - return _POOL_EXECUTOR_CLASS - - -def _make_pool_executor(pool_executor_class: _EXECUTOR_TYPE, max_workers: int) -> concurrent.futures.Executor: - """Create a pool executor of the requested type and size. - - Parameters - ---------- - pool_executor_class : `type` [ `concurrent.futures.Executor` ] - Type of executor pool to create. - max_workers : `int` - Number of workers the pool should use. - - Returns - ------- - executor : `concurrent.futures.Executor` - The new executor. - - Notes - ----- - A process pool marks each of its workers so that parallel operations - running inside a worker use a single worker of their own. A thread pool - must not be marked, since its threads share that state with the process - that created them. - """ - if issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - return pool_executor_class(max_workers=max_workers, initializer=_init_pool_worker) - return pool_executor_class(max_workers=max_workers) - - -# One process pool, identified by executor class and worker count. Starting one -# costs a full interpreter startup per worker under the spawn start method. -# That cost scales with how much the parent process has imported, so we keep -# the pool alive for reuse by later bulk operations. Callers must therefore -# size a pool from the scheme and the configuration alone, and never from the -# number of URIs in hand, since a size that varies from call to call would -# replace the cached pool on every call. Asking for more workers than there is -# work to give them is harmless: only the fork start method starts them all up -# front, and there a worker is cheap. Thread pools are not cached; they cost -# almost nothing to create and holding one open would keep its threads alive -# for no benefit. -_POOL_EXECUTOR_CACHE: tuple[_EXECUTOR_TYPE, int, concurrent.futures.Executor] | None = None - - -def _clear_pool_executor_cache() -> None: - """Shut down and forget the cached pool executor.""" - global _POOL_EXECUTOR_CACHE - cached = _POOL_EXECUTOR_CACHE - _POOL_EXECUTOR_CACHE = None - if cached is not None: - _, _, executor = cached - executor.shutdown(wait=True) - - -def _forget_pool_executor_cache() -> None: - """Forget the cached pool executor without shutting it down. - - Notes - ----- - For use in a child process after a fork, where the inherited executors - refer to worker processes belonging to the parent and must not be driven - or shut down from here. - """ - global _POOL_EXECUTOR_CACHE - _POOL_EXECUTOR_CACHE = None - - -atexit.register(_clear_pool_executor_cache) -if hasattr(os, "register_at_fork"): - os.register_at_fork(after_in_child=_forget_pool_executor_cache) - - -@contextlib.contextmanager -def _pool_executor( - pool_executor_class: _EXECUTOR_TYPE, max_workers: int -) -> Generator[concurrent.futures.Executor]: - """Provide a pool executor of the requested type and size. - - Parameters - ---------- - pool_executor_class : `type` [ `concurrent.futures.Executor` ] - Type of executor pool to use. - max_workers : `int` - Number of workers the pool should use. - - Yields - ------ - executor : `concurrent.futures.Executor` - The executor to submit work to. - - Notes - ----- - A process pool outlives the block and is reused by later calls, so its - workers pay their startup cost once. A different executor class or worker - count replaces the cached pool. A pool that has broken is discarded - so that the next caller is given a fresh one. A thread pool is created for - the block and shut down when it ends. - """ - global _POOL_EXECUTOR_CACHE - if not issubclass(pool_executor_class, concurrent.futures.ProcessPoolExecutor): - with _make_pool_executor(pool_executor_class, max_workers) as transient: - yield transient - return - - cached = _POOL_EXECUTOR_CACHE - if cached is None or cached[:2] != (pool_executor_class, max_workers): - _clear_pool_executor_cache() - executor = _make_pool_executor(pool_executor_class, max_workers) - _POOL_EXECUTOR_CACHE = (pool_executor_class, max_workers, executor) - else: - executor = cached[2] - try: - yield executor - except concurrent.futures.BrokenExecutor: - _discard_pool_executor(executor) - raise - - -def _discard_pool_executor(executor: concurrent.futures.Executor) -> None: - """Drop a cached pool executor so that the next caller gets a fresh one. - - Parameters - ---------- - executor : `concurrent.futures.Executor` - Broken executor. Only clears the cache if it still holds this pool. - """ - global _POOL_EXECUTOR_CACHE - if _POOL_EXECUTOR_CACHE is not None and _POOL_EXECUTOR_CACHE[2] is executor: - _POOL_EXECUTOR_CACHE = None - executor.shutdown(wait=False) - - @dataclasses.dataclass(frozen=True) class ResourceInfo: """Information about this resource.""" @@ -1149,40 +974,8 @@ def _mexists( Implementation helper method for `mexists`. - - Parameters - ---------- - uris : iterable of `ResourcePath` - The URIs to test. - num_workers : `int` or `None`, optional - The number of parallel workers to use when checking for existence - If `None`, the default value will be taken from the environment. - - Returns - ------- - existence : `dict` of [`ResourcePath`, `bool`] - Mapping of original URI to boolean indicating existence. - """ - pool_executor_class = _get_executor_class() - return cls._mexists_pool(pool_executor_class, uris, num_workers=num_workers) - - @classmethod - def _mexists_pool( - cls, - pool_executor_class: _EXECUTOR_TYPE, - uris: Iterable[ResourcePath], - *, - num_workers: int | None = None, - ) -> dict[ResourcePath, bool]: - """Check for existence of multiple URIs at once using specified pool - executor. - - Implementation helper method for `_mexists`. - Parameters ---------- - pool_executor_class : `type` [ `concurrent.futures.Executor` ] - Type of executor pool to use. uris : iterable of `ResourcePath` The URIs to test. num_workers : `int` or `None`, optional @@ -1200,21 +993,18 @@ def _mexists_pool( if not chunks: return {} if len(chunks) == 1: - # Not enough work to be worth handing to another thread or - # process. + # Not enough work to be worth handing to another thread. return cls._exists_chunk(chunks[0]) results: dict[ResourcePath, bool] = {} - with _pool_executor(pool_executor_class, max_workers) as exists_executor: + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as exists_executor: future_exists = {exists_executor.submit(cls._exists_chunk, chunk): chunk for chunk in chunks} for future in concurrent.futures.as_completed(future_exists): try: results.update(future.result()) - except Exception as e: - # The chunk failed as a whole, for example because a - # worker died. - if isinstance(e, concurrent.futures.BrokenExecutor): - _discard_pool_executor(exists_executor) + except Exception: + # The chunk failed as a whole, for example because the + # pool could not start a thread. for uri in future_exists[future]: results[uri] = False return results @@ -1281,55 +1071,8 @@ def mtransfer( whether the transfer succeeded for the target URI. If ``do_raise`` is `True`, this will only be returned if there are no errors. """ - pool_executor_class = _get_executor_class() - return cls._mtransfer( - pool_executor_class, - transfer, - from_to, - overwrite=overwrite, - transaction=transaction, - do_raise=do_raise, - ) - - @classmethod - def _mtransfer( - cls, - pool_executor_class: _EXECUTOR_TYPE, - transfer: str, - from_to: Iterable[tuple[ResourcePath, ResourcePath]], - overwrite: bool = False, - transaction: TransactionProtocol | None = None, - do_raise: bool = True, - ) -> dict[ResourcePath, MBulkResult]: - """Transfer many files in bulk. - - Parameters - ---------- - transfer : `str` - Mode to use for transferring the resource. Generically there are - many standard options: copy, link, symlink, hardlink, relsymlink. - Not all URIs support all modes. - from_to : `list` [ `tuple` [ `ResourcePath`, `ResourcePath` ] ] - A sequence of the source URIs and the target URIs. - overwrite : `bool`, optional - Allow an existing file to be overwritten. Defaults to `False`. - transaction : `~lsst.resources.utils.TransactionProtocol`, optional - A transaction object that can (depending on implementation) - rollback transfers on error. Not guaranteed to be implemented. - The transaction object must be thread safe. - do_raise : `bool`, optional - If `True` an `ExceptionGroup` will be raised containing any - exceptions raised by the individual transfers. Else a dict - reporting the status of each `ResourcePath` will be returned. - - Returns - ------- - copy_status : `dict` [ `ResourcePath`, `MBulkResult` ] - A dict of all the transfer attempts with a value indicating - whether the transfer succeeded for the target URI. - """ max_workers = _get_num_workers() - with _pool_executor(pool_executor_class, max_workers) as transfer_executor: + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as transfer_executor: future_transfers = { transfer_executor.submit( to_uri.transfer_from, @@ -1348,8 +1091,6 @@ def _mtransfer( try: future.result() except Exception as e: - if isinstance(e, concurrent.futures.BrokenExecutor): - _discard_pool_executor(transfer_executor) transferred = MBulkResult(False, e) failed = True else: @@ -1407,11 +1148,6 @@ def mremove( return results - @classmethod - def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResult]: - """Remove multiple URIs using futures.""" - return cls._mremove_pool(_get_executor_class(), uris) - @classmethod def _chunk_uris(cls, uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: """Split URIs into batches sized for the given number of workers. @@ -1467,35 +1203,39 @@ def _remove_chunk(cls, uris: tuple[ResourcePath, ...]) -> dict[ResourcePath, MBu return results @classmethod - def _mremove_pool( - cls, - pool_executor_class: _EXECUTOR_TYPE, - uris: Iterable[ResourcePath], - *, - num_workers: int | None = None, - ) -> dict[ResourcePath, MBulkResult]: - """Remove URIs using a futures pool.""" + def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResult]: + """Remove multiple URIs using threads. + + Implementation helper method for `mremove`. + + Parameters + ---------- + uris : iterable of `ResourcePath` + The URIs to remove. + + Returns + ------- + removal : `dict` of [`ResourcePath`, `MBulkResult`] + Mapping of original URI to the result of removing it. + """ uri_list = list(uris) - max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) + max_workers = _get_num_workers(cls._max_workers) chunks = cls._chunk_uris(uri_list, max_workers) if not chunks: return {} if len(chunks) == 1: - # Not enough work to be worth handing to another thread or - # process. + # Not enough work to be worth handing to another thread. return cls._remove_chunk(chunks[0]) results: dict[ResourcePath, MBulkResult] = {} - with _pool_executor(pool_executor_class, max_workers) as remove_executor: + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as remove_executor: future_remove = {remove_executor.submit(cls._remove_chunk, chunk): chunk for chunk in chunks} for future in concurrent.futures.as_completed(future_remove): try: results.update(future.result()) except Exception as e: - # The chunk failed as a whole, for example because a - # worker died. - if isinstance(e, concurrent.futures.BrokenExecutor): - _discard_pool_executor(remove_executor) + # The chunk failed as a whole, for example because the + # pool could not start a thread. for uri in future_remove[future]: results[uri] = MBulkResult(False, e) return results diff --git a/python/lsst/resources/s3.py b/python/lsst/resources/s3.py index d32952c3..80366691 100644 --- a/python/lsst/resources/s3.py +++ b/python/lsst/resources/s3.py @@ -35,15 +35,7 @@ from ._resourceHandles._baseResourceHandle import ResourceHandleProtocol from ._resourceHandles._s3ResourceHandle import S3ResourceHandle -from ._resourcePath import ( - _EXECUTOR_TYPE, - MBulkResult, - ResourceInfo, - ResourcePath, - _discard_pool_executor, - _get_executor_class, - _pool_executor, -) +from ._resourcePath import MBulkResult, ResourceInfo, ResourcePath from .s3utils import ( _get_s3_connection_parameters, _s3_disable_bucket_validation, @@ -296,20 +288,14 @@ def _mremove_select(cls, chunks: list[tuple[ResourcePath, ...]]) -> dict[Resourc if len(chunks) == 1: # Do the removal directly without futures. return cls._delete_objects_wrapper(chunks[0]) - return cls._mremove_with_pool(_get_executor_class(), chunks) + return cls._mremove_with_pool(chunks) @classmethod - def _mremove_with_pool( - cls, - pool_executor_class: _EXECUTOR_TYPE, - chunks: list[tuple[ResourcePath, ...]], - *, - num_workers: int | None = None, - ) -> dict[ResourcePath, MBulkResult]: + def _mremove_with_pool(cls, chunks: list[tuple[ResourcePath, ...]]) -> dict[ResourcePath, MBulkResult]: # Different name because different API to base class. - max_workers = num_workers if num_workers is not None else _get_num_workers() + max_workers = _get_num_workers(cls._max_workers) results: dict[ResourcePath, MBulkResult] = {} - with _pool_executor(pool_executor_class, max_workers) as remove_executor: + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as remove_executor: future_remove = { remove_executor.submit(cls._delete_objects_wrapper, chunk): i for i, chunk in enumerate(chunks) @@ -319,8 +305,6 @@ def _mremove_with_pool( results.update(future.result()) except Exception as e: # The chunk utterly failed. - if isinstance(e, concurrent.futures.BrokenExecutor): - _discard_pool_executor(remove_executor) chunk = chunks[future_remove[future]] for uri in chunk: results[uri] = MBulkResult(False, e) diff --git a/python/lsst/resources/utils.py b/python/lsst/resources/utils.py index 0812b209..14f1e1f0 100644 --- a/python/lsst/resources/utils.py +++ b/python/lsst/resources/utils.py @@ -244,26 +244,6 @@ def _get_int_env_var(env_var: str) -> int | None: return int_value -# True in processes started as pool workers. Only ever written by -# _init_pool_worker, which runs once per worker process, so the reads need no -# locking. -_IS_POOL_WORKER = False - - -def _init_pool_worker() -> None: - """Mark this process as a pool worker. - - Notes - ----- - Used as the ``initializer`` of a `~concurrent.futures.ProcessPoolExecutor` - so that parallel operations running inside a worker do not spawn workers - of their own. Must not be used with a thread pool, since threads share - this global with the process that created them. - """ - global _IS_POOL_WORKER - _IS_POOL_WORKER = True - - @cache def _get_configured_num_workers() -> int | None: """Return the explicitly requested number of workers. @@ -303,13 +283,10 @@ def _get_num_workers(max_workers: int = MAX_WORKERS) -> int: Returns ------- num : `int` - The number of workers to use. A pool worker always reports one, so - that nested parallel operations do not multiply. Otherwise the value - of ``$LSST_RESOURCES_NUM_WORKERS`` is used if set, and the CPU count - plus two bounded by ``max_workers`` if not. + The number of workers to use. The value of + ``$LSST_RESOURCES_NUM_WORKERS`` is used if set, and the CPU count plus + two bounded by ``max_workers`` if not. """ - if _IS_POOL_WORKER: - return 1 configured = _get_configured_num_workers() if configured is not None: # An explicit request is honored without capping. diff --git a/tests/test_file.py b/tests/test_file.py index 219c5f37..ddc8b277 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -9,7 +9,6 @@ # Use of this source code is governed by a 3-clause BSD-style # license that can be found in the LICENSE file. -import concurrent.futures import contextlib import datetime import os @@ -17,25 +16,11 @@ import unittest import unittest.mock import urllib.parse -from typing import Any -import lsst.resources._resourcePath as resource_path from lsst.resources import ResourceInfo, ResourcePath, ResourcePathExpression from lsst.resources.file import FileResourcePath from lsst.resources.tests import GenericReadWriteTestCase, GenericTestCase -from lsst.resources.utils import ( - _get_configured_num_workers, - _get_default_num_workers, - makeTestTempDir, - removeTestTempDir, -) - - -def _clear_worker_caches() -> None: - """Discard memoized worker-count lookups.""" - _get_configured_num_workers.cache_clear() - _get_default_num_workers.cache_clear() - +from lsst.resources.utils import makeTestTempDir, removeTestTempDir TESTDIR = os.path.abspath(os.path.dirname(__file__)) @@ -265,24 +250,6 @@ def _test_with_restrictive_umask(self, callback): mode = os.stat(dir).st_mode self.assertEqual(mode & TEST_UMASK, 0o0300, f"Permissions incorrect for {dir}: {mode:o}") - @unittest.mock.patch("lsst.resources._resourcePath._POOL_EXECUTOR_CLASS", None) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_EXECUTOR": "process"}) - def test_mexists_process(self) -> None: - """Test mexists with override executor pool. - - Force test with process pool. - """ - super().test_mexists() - - @unittest.mock.patch("lsst.resources._resourcePath._POOL_EXECUTOR_CLASS", None) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_EXECUTOR": "process"}) - def test_mtransfer_process(self) -> None: - """Test transfer with override executor pool. - - Force test with process pool. - """ - super().test_mtransfer() - class RemoveChunkTestCase(unittest.TestCase): """Tests for batched removal.""" @@ -368,74 +335,9 @@ def flaky(self: FileResourcePath) -> bool: # The failure is reported as absent and the rest are still checked. self.assertEqual(results, {uris[0]: True, uris[1]: False, uris[2]: True}) - @unittest.mock.patch.dict(os.environ, {}, clear=False) - @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) - def test_mexists_scheme_cap_sizes_the_pool(self) -> None: - os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) - _clear_worker_caches() - recorded: list[int] = [] - - class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): - def __init__(self, max_workers: int, **kwargs: Any) -> None: - recorded.append(max_workers) - super().__init__(max_workers=max_workers, **kwargs) - - uris = [self.tmpdir.join(f"x{n}.txt") for n in range(20)] - for uri in uris: - uri.write(b"") - - results = FileResourcePath._mexists_pool(_RecordingExecutor, uris) - - self.assertEqual(recorded, [3]) - self.assertTrue(all(results.values())) - def test_empty_existence_check_is_a_no_op(self) -> None: self.assertEqual(ResourcePath.mexists([]), {}) - def test_small_batches_avoid_the_pool(self) -> None: - present = [self.tmpdir.join(f"s{n}.txt") for n in range(3)] - for uri in present: - uri.write(b"") - absent = self.tmpdir.join("nothere.txt") - - def refuse(*args: Any, **kwargs: Any) -> None: - raise AssertionError("a batch this small must not be given to a pool") - - with unittest.mock.patch.object(resource_path, "_pool_executor", refuse): - existence = FileResourcePath._mexists_pool( - concurrent.futures.ThreadPoolExecutor, [*present, absent] - ) - removals = FileResourcePath._mremove_pool( - concurrent.futures.ThreadPoolExecutor, [*present, absent] - ) - - self.assertEqual(existence, {**dict.fromkeys(present, True), absent: False}) - self.assertTrue(all(removals[uri].success for uri in present)) - self.assertFalse(removals[absent].success) - - @unittest.mock.patch.dict(os.environ, {}, clear=False) - @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) - def test_scheme_cap_sizes_the_pool(self) -> None: - os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) - _clear_worker_caches() - recorded: list[int] = [] - - class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): - def __init__(self, max_workers: int, **kwargs: Any) -> None: - recorded.append(max_workers) - super().__init__(max_workers=max_workers, **kwargs) - - uris = [self.tmpdir.join(f"f{n}.txt") for n in range(20)] - for uri in uris: - uri.write(b"") - - results = FileResourcePath._mremove_pool(_RecordingExecutor, uris) - - self.assertEqual(recorded, [3]) - self.assertTrue(all(r.success for r in results.values())) - @contextlib.contextmanager def _override_umask(temp_umask): diff --git a/tests/test_s3.py b/tests/test_s3.py index 9791cf41..9e1952f9 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -274,33 +274,6 @@ def test_fsspec_constructor(self) -> None: def test_fsspec(self) -> None: raise unittest.SkipTest("fsspec s3fs incompatible with moto") - @unittest.mock.patch("lsst.resources._resourcePath._POOL_EXECUTOR_CLASS", None) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_EXECUTOR": "threads"}) - def test_mexists(self) -> None: - """Test mexists with override executor pool. - - moto does not work with process pool. - """ - super().test_mexists() - - @unittest.mock.patch("lsst.resources._resourcePath._POOL_EXECUTOR_CLASS", None) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_EXECUTOR": "threads"}) - def test_mtransfer(self) -> None: - """Test mtransfer with override executor pool. - - moto does not work with process pool. - """ - super().test_mtransfer() - - @unittest.mock.patch("lsst.resources._resourcePath._POOL_EXECUTOR_CLASS", None) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_EXECUTOR": "threads"}) - def test_local_mtransfer(self) -> None: - """Test local mtransfer with override executor pool. - - moto does not work with process pool. - """ - super().test_local_mtransfer() - @unittest.skipIf(not boto3, "Warning: boto3 AWS SDK not found!") class S3ReadWriteTestCase(S3ReadWriteTestCaseBase, unittest.TestCase): diff --git a/tests/test_utils.py b/tests/test_utils.py index fd472f86..74988710 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -9,23 +9,11 @@ # Use of this source code is governed by a 3-clause BSD-style # license that can be found in the LICENSE file. -import concurrent.futures -import multiprocessing import os -import subprocess -import sys import unittest import unittest.mock -from concurrent.futures.process import BrokenProcessPool -from typing import Any -import lsst.resources._resourcePath as resource_path from lsst.resources import ResourcePath -from lsst.resources._resourcePath import ( - _clear_pool_executor_cache, - _make_pool_executor, - _pool_executor, -) from lsst.resources.file import FileResourcePath from lsst.resources.s3 import S3ResourcePath from lsst.resources.utils import ( @@ -33,7 +21,6 @@ _get_configured_num_workers, _get_default_num_workers, _get_num_workers, - _init_pool_worker, ) @@ -65,70 +52,11 @@ def test_explicit_request_bypasses_cap(self) -> None: self.assertEqual(_get_num_workers(), 99) self.assertEqual(_get_num_workers(2), 99) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_NUM_WORKERS": "99"}) - @unittest.mock.patch("lsst.resources.utils._IS_POOL_WORKER", True) - def test_pool_worker_uses_one_worker(self) -> None: - _clear_worker_caches() - self.assertEqual(_get_num_workers(), 1) - self.assertEqual(_get_num_workers(99), 1) - def test_docstring_is_present(self) -> None: # An f-string in the leading position is not a docstring. self.assertIsNotNone(_get_num_workers.__doc__) -class PoolExecutorTestCase(unittest.TestCase): - """Tests for worker-count propagation into pool executors.""" - - def setUp(self) -> None: - _clear_worker_caches() - - def tearDown(self) -> None: - _clear_worker_caches() - - def test_process_worker_reports_one_worker(self) -> None: - # The fork start method is the one where a child inherits the parent's - # memoized state, so both methods must be checked. - for method in ("fork", "spawn"): - if method not in multiprocessing.get_all_start_methods(): - continue - with self.subTest(start_method=method): - parent_before = _get_num_workers() - context = multiprocessing.get_context(method) - with concurrent.futures.ProcessPoolExecutor( - max_workers=2, - mp_context=context, - initializer=_init_pool_worker, - ) as executor: - # The callable has to come from an installed module. The - # spawn start method pickles it by reference and the child - # process cannot import this test module. - observed = list(executor.map(_get_num_workers, [MAX_WORKERS] * 4)) - self.assertEqual(observed, [1, 1, 1, 1]) - self.assertEqual(_get_num_workers(), parent_before) - - def test_thread_pool_does_not_mark_the_parent(self) -> None: - parent_before = _get_num_workers() - executor = _make_pool_executor(concurrent.futures.ThreadPoolExecutor, 2) - with executor: - observed = list(executor.map(_get_num_workers, [MAX_WORKERS] * 4)) - self.assertEqual(observed, [parent_before] * 4) - self.assertEqual(_get_num_workers(), parent_before) - - def test_process_pool_receives_the_requested_size(self) -> None: - recorded: list[int] = [] - - class _RecordingExecutor(concurrent.futures.ProcessPoolExecutor): - def __init__(self, max_workers: int, **kwargs: Any) -> None: - recorded.append(max_workers) - super().__init__(max_workers=max_workers, **kwargs) - - # A cold cache must not cause the parent pool to shrink to one. - _clear_worker_caches() - _make_pool_executor(_RecordingExecutor, 7).shutdown() - self.assertEqual(recorded, [7]) - - class WorkerCapTestCase(unittest.TestCase): """Tests for per-scheme worker caps.""" @@ -150,176 +78,11 @@ def test_an_overridden_cap_is_honored(self) -> None: _clear_worker_caches() self.assertEqual(_get_num_workers(FileResourcePath._max_workers), 2) - @unittest.mock.patch.dict(os.environ, {}, clear=False) - def test_cap_limits_the_default(self) -> None: - os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) - _clear_worker_caches() - self.assertLessEqual(_get_num_workers(S3ResourcePath._max_workers), S3ResourcePath._max_workers) - @unittest.mock.patch.dict(os.environ, {"LSST_RESOURCES_NUM_WORKERS": "99"}) def test_explicit_request_overrides_the_scheme_cap(self) -> None: _clear_worker_caches() self.assertEqual(_get_num_workers(S3ResourcePath._max_workers), 99) -class PoolReuseTestCase(unittest.TestCase): - """Tests for reuse of process pools across calls.""" - - def tearDown(self) -> None: - _clear_pool_executor_cache() - - def test_process_pools_are_reused(self) -> None: - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as first: - pass - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as second: - pass - self.assertIs(first, second) - # Still usable after both blocks have exited. - self.assertEqual(list(second.map(int, ["1", "2"])), [1, 2]) - - def test_different_sizes_replace_the_cached_pool(self) -> None: - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as small: - self.assertEqual(small.submit(int, "1").result(), 1) - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 3) as large: - self.assertEqual(large.submit(int, "2").result(), 2) - self.assertIsNot(small, large) - with self.assertRaises(RuntimeError): - small.submit(int, "1") - # Returning to an earlier size creates a new pool, and also shuts - # down the larger one instead of leaving its workers alive. - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: - self.assertIsNot(replacement, small) - self.assertEqual(replacement.submit(int, "3").result(), 3) - with self.assertRaises(RuntimeError): - large.submit(int, "1") - - @unittest.mock.patch.dict(os.environ, {}, clear=False) - @unittest.mock.patch.object(FileResourcePath, "_max_workers", 3) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) - def test_batch_size_does_not_replace_the_cached_pool(self) -> None: - os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) - _clear_worker_caches() - _clear_pool_executor_cache() - few = [ResourcePath(__file__).updatedFile(f"missing{n}.txt") for n in range(2)] - many = [ResourcePath(__file__).updatedFile(f"missing{n}.txt") for n in range(64)] - - # A batch that occupies only a couple of workers must still be given a - # pool sized for the scheme, or the next larger batch would replace it. - for uris in (few, many, few): - FileResourcePath._mexists_pool(concurrent.futures.ProcessPoolExecutor, uris) - cached = resource_path._POOL_EXECUTOR_CACHE - assert cached is not None - self.assertEqual(cached[1], 3) - if uris is few: - first = cached[2] - self.assertIs(resource_path._POOL_EXECUTOR_CACHE[2], first) - - @unittest.mock.patch.dict(os.environ, {}, clear=False) - def test_s3_batch_size_does_not_size_the_pool(self) -> None: - os.environ.pop("LSST_RESOURCES_NUM_WORKERS", None) - _clear_worker_caches() - recorded: list[int] = [] - - class _RecordingExecutor(concurrent.futures.ThreadPoolExecutor): - def __init__(self, max_workers: int, **kwargs: Any) -> None: - recorded.append(max_workers) - super().__init__(max_workers=max_workers, **kwargs) - - uri = ResourcePath("s3://bucket/object.txt") - with unittest.mock.patch.object(S3ResourcePath, "_delete_objects_wrapper", return_value={}): - S3ResourcePath._mremove_with_pool(_RecordingExecutor, [(uri,)]) - self.assertEqual(recorded, [_get_num_workers()]) - - def test_thread_pools_are_not_reused(self) -> None: - with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as first: - pass - with _pool_executor(concurrent.futures.ThreadPoolExecutor, 2) as second: - pass - self.assertIsNot(first, second) - # A thread pool is cheap, so it is shut down when the block ends. - with self.assertRaises(RuntimeError): - first.submit(int, "1") - - def test_clearing_the_cache_shuts_pools_down(self) -> None: - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as executor: - pass - _clear_pool_executor_cache() - with self.assertRaises(RuntimeError): - executor.submit(int, "1") - # The next request builds a fresh pool. - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: - self.assertIsNot(replacement, executor) - - def test_a_broken_pool_is_replaced(self) -> None: - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as executor: - # Workers start lazily, so submit before there is anything to kill. - list(executor.map(int, ["1", "2"])) - # Kill the workers so the pool is unusable. - for process in list(executor._processes.values()): - process.terminate() - process.join() - with self.assertRaises(concurrent.futures.BrokenExecutor): - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as broken: - broken.submit(int, "1").result() - with _pool_executor(concurrent.futures.ProcessPoolExecutor, 2) as replacement: - self.assertIsNot(replacement, executor) - self.assertEqual(replacement.submit(int, "1").result(), 1) - - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) - def test_bulk_operations_discard_broken_pools(self) -> None: - uris = [ResourcePath(__file__), ResourcePath(__file__).updatedFile("missing.txt")] - executor_class = concurrent.futures.ProcessPoolExecutor - operations = { - "mexists": lambda: FileResourcePath._mexists_pool(executor_class, uris, num_workers=2), - "mremove": lambda: FileResourcePath._mremove_pool(executor_class, uris, num_workers=2), - "mtransfer": lambda: ResourcePath._mtransfer( - executor_class, "copy", [(uris[0], uris[1])], do_raise=False - ), - "s3_mremove": lambda: S3ResourcePath._mremove_with_pool( - executor_class, [(uris[0],), (uris[1],)], num_workers=2 - ), - } - for name, operation in operations.items(): - with self.subTest(operation=name): - with _pool_executor(executor_class, 2) as broken: - pass - - def fail_submission(*args: Any, **kwargs: Any) -> concurrent.futures.Future: - future = concurrent.futures.Future() - future.set_exception(BrokenProcessPool("worker died")) - return future - - # Fail the futures, not submit(), to exercise the exceptions - # caught inside each bulk operation's result loop. - with ( - unittest.mock.patch.object(broken, "submit", side_effect=fail_submission), - unittest.mock.patch("lsst.resources._resourcePath._get_num_workers", return_value=2), - ): - results = operation() - if name == "mexists": - self.assertTrue(all(value is False for value in results.values())) - else: - self.assertTrue(all(not value.success for value in results.values())) - with _pool_executor(executor_class, 2) as replacement: - self.assertIsNot(replacement, broken) - self.assertEqual(replacement.submit(int, "1").result(), 1) - - def test_import_without_fork_support(self) -> None: - # Load dependencies before hiding the hook: POSIX versions of some - # stdlib modules (such as random) assume the hook is available. - subprocess.run( - [ - sys.executable, - "-c", - "import os, importlib; " - "import lsst.resources._resourcePath as resource_path; " - "hasattr(os, 'register_at_fork') and delattr(os, 'register_at_fork'); " - "importlib.reload(resource_path)", - ], - check=True, - capture_output=True, - ) - - if __name__ == "__main__": unittest.main() From 65a748e72316ccc380fdf47a7c0169322fb8081a Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 12:40:50 -0700 Subject: [PATCH 16/20] Send batches of files to each transfer worker mtransfer submitted one task per file and sized its pool from the global default. It now groups by the target scheme, so each scheme's own worker bound applies, and gives each worker a batch. Batches are not floored at _min_chunk_size. That floor is for operations cheap enough that handing one over costs more than doing it, which is never true of a transfer, and it would leave workers idle when a few large files are transferred. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.perf.rst | 6 +- python/lsst/resources/_resourcePath.py | 177 +++++++++++++++++++------ tests/test_file.py | 148 +++++++++++++++------ 3 files changed, 247 insertions(+), 84 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 5fd22f11..906dbfcd 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,4 +1,4 @@ -``ResourcePath.mremove()`` and ``ResourcePath.mexists()`` now send a batch of URIs to each worker rather than submitting one task per URI. -The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it. -A batch of URIs too small to be worth spreading over workers is now checked or removed in the calling thread, since handing a couple of local file checks to a thread costs more than doing them. +``ResourcePath.mremove()``, ``ResourcePath.mexists()`` and ``ResourcePath.mtransfer()`` now send a batch of URIs to each worker rather than submitting one task per URI. +The number of workers is now bounded per scheme rather than globally, so a scheme that holds no connection pool can raise the bound, and ``ResourcePath.mtransfer()`` now uses the bound for the target scheme instead of the global default. +A batch of URIs too small to be worth spreading over workers is now handled in the calling thread, since handing a couple of local file checks to a thread costs more than doing them. The size below which this applies is set per scheme, because a scheme whose every operation is a network round trip benefits from overlapping even two URIs. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index e8952fec..01a8129f 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -43,7 +43,7 @@ AbstractFileSystem = type from collections.abc import Generator, Iterable, Iterator -from typing import Any, Literal, NamedTuple, overload +from typing import Any, Literal, NamedTuple, TypeVar, overload from lsst.utils.iteration import chunk_iterable @@ -66,6 +66,8 @@ # keeps one slow chunk from stalling a worker for the rest of the run. CHUNKS_PER_WORKER = 4 +_T = TypeVar("_T") + class MBulkResult(NamedTuple): """Report on a bulk operation.""" @@ -989,7 +991,7 @@ def _mexists( """ uri_list = list(uris) max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) - chunks = cls._chunk_uris(uri_list, max_workers) + chunks = cls._chunk_work(uri_list, max_workers, cls._min_chunk_size) if not chunks: return {} if len(chunks) == 1: @@ -1071,38 +1073,132 @@ def mtransfer( whether the transfer succeeded for the target URI. If ``do_raise`` is `True`, this will only be returned if there are no errors. """ - max_workers = _get_num_workers() + # A transfer is driven by the target, so group by the target scheme + # and let each scheme decide how many workers to use. + grouped: dict[type[ResourcePath], list[tuple[ResourcePath, ResourcePath]]] = defaultdict(list) + for from_uri, to_uri in from_to: + grouped[to_uri.__class__].append((from_uri, to_uri)) + + results: dict[ResourcePath, MBulkResult] = {} + for uri_class, group in grouped.items(): + results.update( + uri_class._mtransfer(transfer, group, overwrite=overwrite, transaction=transaction) + ) + + if do_raise and any(not res.success for res in results.values()): + raise ExceptionGroup( + f"Errors transferring {len(results)} artifacts", + tuple(res.exception for res in results.values() if res.exception is not None), + ) + + return results + + @classmethod + def _mtransfer( + cls, + transfer: str, + from_to: Iterable[tuple[ResourcePath, ResourcePath]], + *, + overwrite: bool = False, + transaction: TransactionProtocol | None = None, + ) -> dict[ResourcePath, MBulkResult]: + """Transfer many files in bulk to targets of this scheme. + + Implementation helper method for `mtransfer`. + + Parameters + ---------- + transfer : `str` + Mode to use for transferring the resource. + from_to : iterable [ `tuple` [ `ResourcePath`, `ResourcePath` ] ] + A sequence of the source URIs and the target URIs. + overwrite : `bool`, optional + Allow an existing file to be overwritten. Defaults to `False`. + transaction : `~lsst.resources.utils.TransactionProtocol`, optional + A transaction object that can (depending on implementation) + rollback transfers on error. Not guaranteed to be implemented. + The transaction object must be thread safe. + + Returns + ------- + copy_status : `dict` [ `ResourcePath`, `MBulkResult` ] + A dict of all the transfer attempts with a value indicating + whether the transfer succeeded for the target URI. + + Notes + ----- + Batches are never floored at ``_min_chunk_size``. That floor exists + for operations cheap enough that handing one over costs more than + doing it, which is never true of a transfer, and a floor would leave + workers idle when a few large files are transferred. + """ + pairs = list(from_to) + max_workers = _get_num_workers(cls._max_workers) + chunks = cls._chunk_work(pairs, max_workers, min_chunk_size=1) + if not chunks: + return {} + if len(chunks) == 1: + # Not enough work to be worth handing to another thread. + return cls._transfer_chunk(chunks[0], transfer, overwrite, transaction) + + results: dict[ResourcePath, MBulkResult] = {} with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as transfer_executor: future_transfers = { - transfer_executor.submit( - to_uri.transfer_from, - from_uri, - transfer=transfer, - overwrite=overwrite, - transaction=transaction, - multithreaded=False, - ): to_uri - for from_uri, to_uri in from_to + transfer_executor.submit(cls._transfer_chunk, chunk, transfer, overwrite, transaction): chunk + for chunk in chunks } - results: dict[ResourcePath, MBulkResult] = {} - failed = False for future in concurrent.futures.as_completed(future_transfers): - to_uri = future_transfers[future] try: - future.result() + results.update(future.result()) except Exception as e: - transferred = MBulkResult(False, e) - failed = True - else: - transferred = MBulkResult(True, None) - results[to_uri] = transferred + # The chunk failed as a whole, for example because the + # pool could not start a thread. + for _, to_uri in future_transfers[future]: + results[to_uri] = MBulkResult(False, e) + return results - if do_raise and failed: - raise ExceptionGroup( - f"Errors transferring {len(results)} artifacts", - tuple(res.exception for res in results.values() if res.exception is not None), - ) + @classmethod + def _transfer_chunk( + cls, + from_to: tuple[tuple[ResourcePath, ResourcePath], ...], + transfer: str, + overwrite: bool, + transaction: TransactionProtocol | None, + ) -> dict[ResourcePath, MBulkResult]: + """Transfer a batch of files, reporting each result independently. + Parameters + ---------- + from_to : `tuple` [ `tuple` [ `ResourcePath`, `ResourcePath` ], ... ] + The source and target URIs to transfer. + transfer : `str` + Mode to use for transferring the resource. + overwrite : `bool` + Allow an existing file to be overwritten. + transaction : `~lsst.resources.utils.TransactionProtocol` or `None` + A transaction object that can (depending on implementation) + rollback transfers on error. + + Returns + ------- + results : `dict` [ `ResourcePath`, `MBulkResult` ] + An entry for every target URI in ``from_to``. A transfer that + fails does not prevent the transfers after it in the batch. + """ + results: dict[ResourcePath, MBulkResult] = {} + for from_uri, to_uri in from_to: + try: + to_uri.transfer_from( + from_uri, + transfer=transfer, + overwrite=overwrite, + transaction=transaction, + multithreaded=False, + ) + except Exception as e: + results[to_uri] = MBulkResult(False, e) + else: + results[to_uri] = MBulkResult(True, None) return results def remove(self) -> None: @@ -1149,33 +1245,34 @@ def mremove( return results @classmethod - def _chunk_uris(cls, uris: list[ResourcePath], max_workers: int) -> list[tuple[ResourcePath, ...]]: - """Split URIs into batches sized for the given number of workers. + def _chunk_work(cls, items: list[_T], max_workers: int, min_chunk_size: int) -> list[tuple[_T, ...]]: + """Split work items into batches sized for the given worker count. Parameters ---------- - uris : `list` [ `ResourcePath` ] - The URIs to split. + items : `list` + The work items to split. max_workers : `int` Number of workers the batches will be spread across. + min_chunk_size : `int` + Smallest batch to create, below which the cost of handing the + batch over exceeds the cost of the work in it. Returns ------- - chunks : `list` [ `tuple` [ `ResourcePath`, ... ] ] - The batches. Empty if ``uris`` is empty. A single batch means the + chunks : `list` [ `tuple` ] + The batches. Empty if ``items`` is empty. A single batch means the work is not worth spreading, and callers run it directly. Notes ----- - Several batches per worker let a worker that draws quick URIs move on - to more of them, but no batch is smaller than ``_min_chunk_size``, - below which the cost of handing the batch over exceeds the cost of - the work in it. + Several batches per worker let a worker that draws quick items move + on to more of them. """ - if not uris: + if not items: return [] - chunk_size = max(cls._min_chunk_size, math.ceil(len(uris) / (max_workers * CHUNKS_PER_WORKER))) - return list(chunk_iterable(uris, chunk_size=chunk_size)) + chunk_size = max(min_chunk_size, math.ceil(len(items) / (max_workers * CHUNKS_PER_WORKER))) + return list(chunk_iterable(items, chunk_size=chunk_size)) @classmethod def _remove_chunk(cls, uris: tuple[ResourcePath, ...]) -> dict[ResourcePath, MBulkResult]: @@ -1220,7 +1317,7 @@ def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResul """ uri_list = list(uris) max_workers = _get_num_workers(cls._max_workers) - chunks = cls._chunk_uris(uri_list, max_workers) + chunks = cls._chunk_work(uri_list, max_workers, cls._min_chunk_size) if not chunks: return {} if len(chunks) == 1: diff --git a/tests/test_file.py b/tests/test_file.py index ddc8b277..34da46c0 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -11,11 +11,15 @@ import contextlib import datetime +import functools import os import pathlib +import threading import unittest import unittest.mock import urllib.parse +from collections.abc import Callable, Iterator +from typing import Any from lsst.resources import ResourceInfo, ResourcePath, ResourcePathExpression from lsst.resources.file import FileResourcePath @@ -251,8 +255,8 @@ def _test_with_restrictive_umask(self, callback): self.assertEqual(mode & TEST_UMASK, 0o0300, f"Permissions incorrect for {dir}: {mode:o}") -class RemoveChunkTestCase(unittest.TestCase): - """Tests for batched removal.""" +class BulkOperationTestCase(unittest.TestCase): + """Tests for batched bulk operations on local files.""" def setUp(self) -> None: self.tmpdir = ResourcePath(makeTestTempDir(TESTDIR), forceDirectory=True) @@ -260,67 +264,72 @@ def setUp(self) -> None: def tearDown(self) -> None: removeTestTempDir(self.tmpdir.ospath) - def test_failure_does_not_abandon_the_rest_of_the_chunk(self) -> None: - uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] + def _make_files(self, prefix: str, count: int) -> list[ResourcePath]: + uris = [self.tmpdir.join(f"{prefix}{n}.txt") for n in range(count)] for uri in uris: uri.write(b"") - # Remove one out from under the batch so that its own removal raises. - uris[1].remove() - - results = FileResourcePath._remove_chunk(tuple(uris)) - - self.assertEqual(len(results), 5) - self.assertFalse(results[uris[1]].success) - self.assertIsInstance(results[uris[1]].exception, FileNotFoundError) - for uri in (uris[0], uris[2], uris[3], uris[4]): - self.assertTrue(results[uri].success, f"{uri} should have been removed") - self.assertFalse(uri.exists()) + return uris @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_chunk_sizes(self) -> None: - uris = [self.tmpdir.join(f"f{n}.txt") for n in range(5)] - # Fewer URIs than chunk slots gives one URI per chunk. - chunks = FileResourcePath._chunk_uris(uris, 32) + items = list(range(5)) + + # Fewer items than chunk slots gives one item per chunk. + chunks = FileResourcePath._chunk_work(items, 32, FileResourcePath._min_chunk_size) self.assertEqual(len(chunks), 5) self.assertTrue(all(len(c) == 1 for c in chunks)) - # More URIs than chunk slots gives evenly sized chunks. - chunks = FileResourcePath._chunk_uris(uris, 1) + # More items than chunk slots gives evenly sized chunks. + chunks = FileResourcePath._chunk_work(items, 1, FileResourcePath._min_chunk_size) self.assertEqual([len(c) for c in chunks], [2, 2, 1]) # An empty input yields no chunks at all. - self.assertEqual(FileResourcePath._chunk_uris([], 4), []) + self.assertEqual(FileResourcePath._chunk_work([], 4, 1), []) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 4) def test_chunk_size_floor(self) -> None: - uris = [self.tmpdir.join(f"f{n}.txt") for n in range(10)] + items = list(range(10)) # A batch that would otherwise be spread thinly is kept in one piece, # which is the signal to the caller to handle it without a pool. - self.assertEqual([len(c) for c in FileResourcePath._chunk_uris(uris[:4], 32)], [4]) + self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items[:4], 32, 4)], [4]) # The floor never makes chunks larger than the worker count calls for. - self.assertEqual([len(c) for c in FileResourcePath._chunk_uris(uris, 1)], [4, 4, 2]) + self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items, 1, 4)], [4, 4, 2]) - def test_empty_removal_is_a_no_op(self) -> None: + def test_empty_bulk_operations_are_no_ops(self) -> None: self.assertEqual(ResourcePath.mremove([]), {}) + self.assertEqual(ResourcePath.mexists([]), {}) + self.assertEqual(ResourcePath.mtransfer("copy", []), {}) - def test_exists_chunk_reports_each_uri(self) -> None: - present = [self.tmpdir.join(f"p{n}.txt") for n in range(3)] - for uri in present: - uri.write(b"") + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + def test_removal_failure_does_not_abandon_the_rest(self) -> None: + uris = self._make_files("f", 20) + # Remove one out from under the batch so that its own removal raises. + uris[1].remove() + + results = ResourcePath.mremove(uris, do_raise=False) + + self.assertEqual(len(results), len(uris)) + self.assertFalse(results[uris[1]].success) + self.assertIsInstance(results[uris[1]].exception, FileNotFoundError) + for uri in uris[2:]: + self.assertTrue(results[uri].success, f"{uri} should have been removed") + self.assertFalse(uri.exists()) + + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + def test_existence_check_reports_each_uri(self) -> None: + present = self._make_files("p", 20) absent = self.tmpdir.join("gone.txt") - results = FileResourcePath._exists_chunk((*present, absent)) + results = ResourcePath.mexists([*present, absent]) - self.assertEqual(len(results), 4) + self.assertEqual(len(results), len(present) + 1) self.assertTrue(all(results[uri] for uri in present)) self.assertFalse(results[absent]) - def test_exists_chunk_treats_an_error_as_missing(self) -> None: - uris = [self.tmpdir.join(f"e{n}.txt") for n in range(3)] - for uri in uris: - uri.write(b"") + @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + def test_existence_check_treats_an_error_as_missing(self) -> None: + uris = self._make_files("e", 20) failing = uris[1].ospath real_exists = FileResourcePath.exists @@ -330,13 +339,70 @@ def flaky(self: FileResourcePath) -> bool: return real_exists(self) with unittest.mock.patch.object(FileResourcePath, "exists", flaky): - results = FileResourcePath._exists_chunk(tuple(uris)) + results = ResourcePath.mexists(uris) # The failure is reported as absent and the rest are still checked. - self.assertEqual(results, {uris[0]: True, uris[1]: False, uris[2]: True}) - - def test_empty_existence_check_is_a_no_op(self) -> None: - self.assertEqual(ResourcePath.mexists([]), {}) + self.assertFalse(results[uris[1]]) + self.assertTrue(all(results[uri] for uri in uris if uri != uris[1])) + + def test_transfer_of_many_files(self) -> None: + sources = self._make_files("src", 50) + for i, src in enumerate(sources): + src.write(f"{i}".encode()) + destinations = [self.tmpdir.join(f"dest{n}.txt") for n in range(len(sources))] + + results = ResourcePath.mtransfer("copy", zip(sources, destinations, strict=True)) + + self.assertEqual(len(results), len(sources)) + self.assertTrue(all(res.success for res in results.values())) + for i, dest in enumerate(destinations): + self.assertEqual(dest.read().decode(), str(i)) + + def test_transfer_failure_does_not_abandon_the_rest(self) -> None: + sources = self._make_files("s", 20) + destinations = [self.tmpdir.join(f"d{n}.txt") for n in range(len(sources))] + # An existing target fails when overwriting is not allowed. + destinations[1].write(b"in the way") + + results = ResourcePath.mtransfer("copy", zip(sources, destinations, strict=True), do_raise=False) + + self.assertEqual(len(results), len(sources)) + self.assertFalse(results[destinations[1]].success) + for dest in destinations[2:]: + self.assertTrue(results[dest].success, f"{dest} should have been written") + self.assertTrue(dest.exists()) + + def test_transfer_registers_undo_actions(self) -> None: + sources = self._make_files("u", 20) + destinations = [self.tmpdir.join(f"undo{n}.txt") for n in range(len(sources))] + transaction = _RecordingTransaction() + + results = ResourcePath.mtransfer( + "copy", zip(sources, destinations, strict=True), overwrite=True, transaction=transaction + ) + + self.assertTrue(all(res.success for res in results.values())) + # Every transfer must be undoable by the caller that supplied the + # transaction, no matter which worker performed it. + self.assertEqual(len(transaction.undone), len(sources)) + + for undo in transaction.undone: + undo() + self.assertFalse(any(dest.exists() for dest in destinations)) + + +class _RecordingTransaction: + """Transaction that collects the undo actions registered against it.""" + + def __init__(self) -> None: + self.undone: list[Callable[[], Any]] = [] + self._lock = threading.Lock() + + @contextlib.contextmanager + def undoWith(self, name: str, undoFunc: Callable, *args: Any, **kwargs: Any) -> Iterator[None]: + yield None + with self._lock: + self.undone.append(functools.partial(undoFunc, *args, **kwargs)) @contextlib.contextmanager From e3327f56141bf32a34599252ae4d57b7590a18c5 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 13:47:29 -0700 Subject: [PATCH 17/20] Give each scheme a fixed batch size The batch size was derived from how many URIs a call was given, which pinned the number of batches and let the batch itself grow without bound. A batch that then drew a run of slow URIs stalled a worker for the rest of the operation with no way to rebalance: with forty slow URIs among forty thousand, a batch of a thousand took 10.3s against a balanced 1.2s, and a batch of twenty-five took 6.5s. Throughput is flat across batch sizes above the point where a batch is worth handing over, so a fixed size per scheme costs nothing and removes the sizing arithmetic. A scheme sets the size from the cost of one of its operations, and transfers get their own because they cost far more than a metadata operation and scale with a file size that is not known up front. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56097.perf.rst | 8 ++-- python/lsst/resources/_resourcePath.py | 64 +++++++++++++------------- python/lsst/resources/file.py | 14 ++++-- tests/test_file.py | 41 ++++++++--------- 4 files changed, 66 insertions(+), 61 deletions(-) diff --git a/doc/changes/DM-56097.perf.rst b/doc/changes/DM-56097.perf.rst index 906dbfcd..97a06ee2 100644 --- a/doc/changes/DM-56097.perf.rst +++ b/doc/changes/DM-56097.perf.rst @@ -1,4 +1,6 @@ ``ResourcePath.mremove()``, ``ResourcePath.mexists()`` and ``ResourcePath.mtransfer()`` now send a batch of URIs to each worker rather than submitting one task per URI. -The number of workers is now bounded per scheme rather than globally, so a scheme that holds no connection pool can raise the bound, and ``ResourcePath.mtransfer()`` now uses the bound for the target scheme instead of the global default. -A batch of URIs too small to be worth spreading over workers is now handled in the calling thread, since handing a couple of local file checks to a thread costs more than doing them. -The size below which this applies is set per scheme, because a scheme whose every operation is a network round trip benefits from overlapping even two URIs. +The batch size is fixed per scheme rather than derived from how many URIs a call is given, so a batch never grows large enough that one worker drawing a run of slow URIs stalls the operation with no way to rebalance. +A scheme sets it from the cost of one of its operations: a local file check is cheap enough that a batch has to reach a thousand URIs before threading it beats a loop in the calling thread, while a scheme whose every operation is a network round trip is worth overlapping immediately. +Transfers batch separately from existence checks and removals, since a transfer costs far more and scales with a file size that is not known in advance. +The upper bound on the number of workers is now set per scheme, so a scheme that holds no connection pool can raise it, and ``ResourcePath.mtransfer()`` now uses the bound for the target scheme instead of the global default. +A batch of URIs that fits in a single chunk is handled in the calling thread rather than being given to a pool. diff --git a/python/lsst/resources/_resourcePath.py b/python/lsst/resources/_resourcePath.py index 01a8129f..abcf4026 100644 --- a/python/lsst/resources/_resourcePath.py +++ b/python/lsst/resources/_resourcePath.py @@ -21,7 +21,6 @@ import io import locale import logging -import math import os import posixpath import re @@ -62,10 +61,6 @@ # Precomputed escaped hash ESCAPED_HASH = urllib.parse.quote("#") -# Chunks to create per worker when batching bulk operations. Oversubscribing -# keeps one slow chunk from stalling a worker for the rest of the run. -CHUNKS_PER_WORKER = 4 - _T = TypeVar("_T") @@ -174,14 +169,24 @@ class ResourcePath: # numpydoc ignore=PR02 sized to match it; schemes with no pool can raise it. """ - _min_chunk_size: int = 1 - """Smallest batch of URIs worth giving to a worker of its own. + _chunk_size: int = 1 + """Number of URIs given to each worker by `mexists` and `mremove`. + + A batch that fits in a single chunk is handled in the calling thread + rather than being sent to a pool. The default suits a scheme where every + operation is a network round trip, which is expensive enough that handing + over one URI at a time costs nothing and gives the pool the freedom to + balance itself. A scheme whose operations are cheap should raise it until + a chunk is worth handing over. + """ + + _transfer_chunk_size: int = 1 + """Number of files given to each worker by `mtransfer`. - A batch no larger than this is handled in the calling thread instead of - being sent to a pool, since handing work to another thread or process - costs more than doing it. The default suits a scheme where every - operation is a network round trip and so is worth overlapping even for a - couple of URIs. A scheme whose operations are cheap should raise it. + Kept separate from ``_chunk_size`` because a transfer costs orders of + magnitude more than an existence check on the same scheme, and its cost + scales with a file size the caller does not know in advance, so chunks + have to stay small enough for the pool queue to balance them. """ # This is not an ABC with abstract methods because the __new__ being @@ -991,7 +996,7 @@ def _mexists( """ uri_list = list(uris) max_workers = num_workers if num_workers is not None else _get_num_workers(cls._max_workers) - chunks = cls._chunk_work(uri_list, max_workers, cls._min_chunk_size) + chunks = cls._chunk_work(uri_list, cls._chunk_size) if not chunks: return {} if len(chunks) == 1: @@ -1124,17 +1129,10 @@ def _mtransfer( copy_status : `dict` [ `ResourcePath`, `MBulkResult` ] A dict of all the transfer attempts with a value indicating whether the transfer succeeded for the target URI. - - Notes - ----- - Batches are never floored at ``_min_chunk_size``. That floor exists - for operations cheap enough that handing one over costs more than - doing it, which is never true of a transfer, and a floor would leave - workers idle when a few large files are transferred. """ pairs = list(from_to) max_workers = _get_num_workers(cls._max_workers) - chunks = cls._chunk_work(pairs, max_workers, min_chunk_size=1) + chunks = cls._chunk_work(pairs, cls._transfer_chunk_size) if not chunks: return {} if len(chunks) == 1: @@ -1244,19 +1242,16 @@ def mremove( return results - @classmethod - def _chunk_work(cls, items: list[_T], max_workers: int, min_chunk_size: int) -> list[tuple[_T, ...]]: - """Split work items into batches sized for the given worker count. + @staticmethod + def _chunk_work(items: list[_T], chunk_size: int) -> list[tuple[_T, ...]]: + """Split work items into batches of a fixed size. Parameters ---------- items : `list` The work items to split. - max_workers : `int` - Number of workers the batches will be spread across. - min_chunk_size : `int` - Smallest batch to create, below which the cost of handing the - batch over exceeds the cost of the work in it. + chunk_size : `int` + Number of items to put in each batch. Returns ------- @@ -1266,12 +1261,15 @@ def _chunk_work(cls, items: list[_T], max_workers: int, min_chunk_size: int) -> Notes ----- - Several batches per worker let a worker that draws quick items move - on to more of them. + The batch size does not depend on how many items there are. Sizing it + from the total would make a batch grow without bound as the total + grows, and a batch that draws a run of slow URIs then stalls a worker + for the rest of the operation with no way to rebalance. Asking for + more batches than there are workers is harmless, since a pool only + starts a thread when there is a batch waiting for it. """ if not items: return [] - chunk_size = max(min_chunk_size, math.ceil(len(items) / (max_workers * CHUNKS_PER_WORKER))) return list(chunk_iterable(items, chunk_size=chunk_size)) @classmethod @@ -1317,7 +1315,7 @@ def _mremove(cls, uris: Iterable[ResourcePath]) -> dict[ResourcePath, MBulkResul """ uri_list = list(uris) max_workers = _get_num_workers(cls._max_workers) - chunks = cls._chunk_work(uri_list, max_workers, cls._min_chunk_size) + chunks = cls._chunk_work(uri_list, cls._chunk_size) if not chunks: return {} if len(chunks) == 1: diff --git a/python/lsst/resources/file.py b/python/lsst/resources/file.py index 93d41b42..7dd80f3a 100644 --- a/python/lsst/resources/file.py +++ b/python/lsst/resources/file.py @@ -84,10 +84,16 @@ class FileResourcePath(ResourcePath): # By definition refers to a local file isLocal = True - # A missing-file check on a local or cluster filesystem takes on the order - # of 100 microseconds, so a batch has to be around this large before - # spreading it over workers beats a plain loop. - _min_chunk_size = 100 + # A warm existence check or removal here costs a few microseconds, almost + # all of it holding the GIL, so a batch has to be this large before + # spreading it over threads beats a plain loop in the calling thread. + # Measurements above this size are flat, so it also matches the 1000 keys + # an S3 bulk delete takes. + _chunk_size = 1000 + + # A transfer costs orders of magnitude more than an existence check and + # scales with the file size, so batches stay small enough to balance. + _transfer_chunk_size = 25 @property def ospath(self) -> str: diff --git a/tests/test_file.py b/tests/test_file.py index 34da46c0..427f37e8 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -270,38 +270,37 @@ def _make_files(self, prefix: str, count: int) -> list[ResourcePath]: uri.write(b"") return uris - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) def test_chunk_sizes(self) -> None: - items = list(range(5)) + items = list(range(10)) - # Fewer items than chunk slots gives one item per chunk. - chunks = FileResourcePath._chunk_work(items, 32, FileResourcePath._min_chunk_size) - self.assertEqual(len(chunks), 5) - self.assertTrue(all(len(c) == 1 for c in chunks)) + # The batch size does not depend on how many items there are. + self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items, 4)], [4, 4, 2]) + self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items, 1)], [1] * 10) - # More items than chunk slots gives evenly sized chunks. - chunks = FileResourcePath._chunk_work(items, 1, FileResourcePath._min_chunk_size) - self.assertEqual([len(c) for c in chunks], [2, 2, 1]) + # A batch that fits in one chunk is the signal to the caller to handle + # it without a pool. + self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items, 100)], [10]) # An empty input yields no chunks at all. - self.assertEqual(FileResourcePath._chunk_work([], 4, 1), []) - - def test_chunk_size_floor(self) -> None: - items = list(range(10)) + self.assertEqual(FileResourcePath._chunk_work([], 4), []) - # A batch that would otherwise be spread thinly is kept in one piece, - # which is the signal to the caller to handle it without a pool. - self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items[:4], 32, 4)], [4]) + def test_schemes_size_their_own_chunks(self) -> None: + # A local operation is cheap enough that a batch has to be large + # before threading it pays, while a scheme whose every operation is a + # round trip is worth overlapping immediately. + self.assertEqual(FileResourcePath._chunk_size, 1000) + self.assertEqual(ResourcePath._chunk_size, 1) - # The floor never makes chunks larger than the worker count calls for. - self.assertEqual([len(c) for c in FileResourcePath._chunk_work(items, 1, 4)], [4, 4, 2]) + # A transfer is far more expensive than an existence check on the same + # scheme, so it batches separately. + self.assertLess(FileResourcePath._transfer_chunk_size, FileResourcePath._chunk_size) def test_empty_bulk_operations_are_no_ops(self) -> None: self.assertEqual(ResourcePath.mremove([]), {}) self.assertEqual(ResourcePath.mexists([]), {}) self.assertEqual(ResourcePath.mtransfer("copy", []), {}) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + @unittest.mock.patch.object(FileResourcePath, "_chunk_size", 1) def test_removal_failure_does_not_abandon_the_rest(self) -> None: uris = self._make_files("f", 20) # Remove one out from under the batch so that its own removal raises. @@ -316,7 +315,7 @@ def test_removal_failure_does_not_abandon_the_rest(self) -> None: self.assertTrue(results[uri].success, f"{uri} should have been removed") self.assertFalse(uri.exists()) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + @unittest.mock.patch.object(FileResourcePath, "_chunk_size", 1) def test_existence_check_reports_each_uri(self) -> None: present = self._make_files("p", 20) absent = self.tmpdir.join("gone.txt") @@ -327,7 +326,7 @@ def test_existence_check_reports_each_uri(self) -> None: self.assertTrue(all(results[uri] for uri in present)) self.assertFalse(results[absent]) - @unittest.mock.patch.object(FileResourcePath, "_min_chunk_size", 1) + @unittest.mock.patch.object(FileResourcePath, "_chunk_size", 1) def test_existence_check_treats_an_error_as_missing(self) -> None: uris = self._make_files("e", 20) failing = uris[1].ospath From 70c42fb9c8383ff526c79998c84a97de812e1b4f Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 14:03:04 -0700 Subject: [PATCH 18/20] Add 3.15 to build matrix --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 49dfc71e..835c2cf2 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -13,7 +13,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python-version: ["3.11", "3.12", "3.13", "3.14"] + python-version: ["3.11", "3.12", "3.13", "3.14", "3.15", "3.15t"] steps: - uses: actions/checkout@v7 From 92634486116e98cb90765e3e137841cf585099bf Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 14:09:00 -0700 Subject: [PATCH 19/20] Skip the WebDAV test packages on the free-threaded build wsgidav needs bcrypt, which publishes free-threaded wheels no newer than cp314t, so this Python builds it from source. That build fails because the pyo3 behind bcrypt's Rust extension refuses a Python newer than 3.13 and cannot fall back to the limited API on a free-threaded build. The WebDAV tests already skip when wsgidav is missing, reporting that neither WsgiDAVApp nor a test endpoint is available. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 835c2cf2..a825bef9 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -40,6 +40,12 @@ jobs: uv pip install --system cryptography - name: Install WebDAV packages for testing + # wsgidav needs bcrypt, which publishes no free-threaded wheel for + # this Python and cannot be built from source because the pyo3 its + # Rust extension uses refuses a Python newer than 3.13 and cannot + # fall back to the limited API on a free-threaded build. The WebDAV + # tests skip when wsgidav is missing. + if: matrix.python-version != '3.15t' run: | uv pip install --system cheroot wsgidav From 221609026b74acf7b6e37aa6bede5185171b868c Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 18 Sep 2026 14:39:38 -0700 Subject: [PATCH 20/20] Skip the concurrent S3 tests on the free-threaded build moto mocks S3 by patching botocore in this process, and its stubber reloads the module holding a backend's URL table on every request. Concurrent reloads of the same module race and the loser raises ImportError, which a bulk operation reports as a URI it could not reach. Only a free-threaded interpreter issues those requests at the same time, and which request loses varies from run to run, so the skip covers every test that drives requests in parallel rather than the one that happened to fail. It is conditioned on the interpreter having no GIL rather than on a version, so it stops applying if moto becomes thread safe. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_s3.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_s3.py b/tests/test_s3.py index 9e1952f9..b8b9fb64 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -11,6 +11,7 @@ import datetime import os +import sys import time import unittest from inspect import signature @@ -22,6 +23,19 @@ from lsst.resources.s3utils import clean_test_environment_for_s3 from lsst.resources.tests import GenericReadWriteTestCase, GenericTestCase +# moto mocks S3 by patching botocore in this process, and its stubber reloads +# the module holding a backend's URL table on every request. Concurrent +# reloads of the same module race, and the loser raises ImportError, which a +# bulk operation reports as a URI that could not be reached. Only a +# free-threaded interpreter issues those requests at the same time, and which +# request loses varies from run to run, so every test that drives requests in +# parallel is affected rather than one in particular. +_MOTO_IS_THREAD_SAFE = getattr(sys, "_is_gil_enabled", lambda: True)() +skip_if_moto_races = unittest.skipIf( + not _MOTO_IS_THREAD_SAFE, + "moto reloads modules per request, which races without the GIL", +) + try: import boto3 import botocore @@ -274,6 +288,20 @@ def test_fsspec_constructor(self) -> None: def test_fsspec(self) -> None: raise unittest.SkipTest("fsspec s3fs incompatible with moto") + # These drive many S3 requests in parallel, so under a free-threaded + # interpreter they are the tests moto's reload race shows up in. + @skip_if_moto_races + def test_mexists(self) -> None: + super().test_mexists() + + @skip_if_moto_races + def test_mtransfer(self) -> None: + super().test_mtransfer() + + @skip_if_moto_races + def test_local_mtransfer(self) -> None: + super().test_local_mtransfer() + @unittest.skipIf(not boto3, "Warning: boto3 AWS SDK not found!") class S3ReadWriteTestCase(S3ReadWriteTestCaseBase, unittest.TestCase):