From 17d90a7efe50920772b1812fe5c262825c28f8fb Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Wed, 26 Nov 2025 10:20:57 +0100 Subject: [PATCH 01/35] adding download task and creating seperate download pool --- openeo/extra/job_management/_manager.py | 45 ++++++++++++++---- openeo/extra/job_management/_thread_worker.py | 46 +++++++++++++++++++ 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 2ce78ba52..d0458ba83 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -32,6 +32,7 @@ from openeo.extra.job_management._thread_worker import ( _JobManagerWorkerThreadPool, _JobStartTask, + _JobDownloadTask ) from openeo.rest import OpenEoApiError from openeo.rest.auth.auth import BearerAuth @@ -172,6 +173,7 @@ def start_job( .. versionchanged:: 0.47.0 Added ``download_results`` parameter. + """ # Expected columns in the job DB dataframes. @@ -219,6 +221,7 @@ def __init__( ) self._thread = None self._worker_pool = None + self._download_pool = None # Generic cache self._cache = {} @@ -351,6 +354,7 @@ def start_job_thread(self, start_job: Callable[[], BatchJob], job_db: JobDatabas self._stop_thread = False self._worker_pool = _JobManagerWorkerThreadPool() + self._download_pool = _JobManagerWorkerThreadPool() def run_loop(): # TODO: support user-provided `stats` @@ -388,7 +392,13 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): .. versionadded:: 0.32.0 """ - self._worker_pool.shutdown() + if self._worker_pool is not None: + self._worker_pool.shutdown() + self._worker_pool = None + + if self._download_pool is not None: + self._download_pool.shutdown() + self._download_pool = None if self._thread is not None: self._stop_thread = True @@ -493,6 +503,8 @@ def run_jobs( stats = collections.defaultdict(int) self._worker_pool = _JobManagerWorkerThreadPool() + self._download_pool = _JobManagerWorkerThreadPool() + while ( sum( @@ -511,7 +523,7 @@ def run_jobs( stats["sleep"] += 1 # TODO; run post process after shutdown once more to ensure completion? - self._worker_pool.shutdown() + self.stop_job_thread() return stats @@ -553,7 +565,11 @@ def _job_update_loop( stats["job_db persist"] += 1 total_added += 1 - self._process_threadworker_updates(self._worker_pool, job_db=job_db, stats=stats) + if self._worker_pool is not None: + self._process_threadworker_updates(worker_pool=self._worker_pool, job_db=job_db, stats=stats) + + if self._download_pool is not None: + self._process_threadworker_updates(worker_pool=self._download_pool, job_db=job_db, stats=stats) # TODO: move this back closer to the `_track_statuses` call above, once job done/error handling is also handled in threads? for job, row in jobs_done: @@ -565,6 +581,7 @@ def _job_update_loop( for job, row in jobs_cancel: self.on_job_cancel(job, row) + def _launch_job(self, start_job, df, i, backend_name, stats: Optional[dict] = None): """Helper method for launching jobs @@ -657,7 +674,7 @@ def _refresh_bearer_token(self, connection: Connection, *, max_age: float = 60) else: _log.warning("Failed to proactively refresh bearer token") - def _process_threadworker_updates( + def _process_task_results( self, worker_pool: _JobManagerWorkerThreadPool, *, @@ -723,15 +740,23 @@ def on_job_done(self, job: BatchJob, row): """ # TODO: param `row` is never accessed in this method. Remove it? Is this intended for future use? if self._download_results: - job_metadata = job.describe() - job_dir = self.get_job_dir(job.job_id) - metadata_path = self.get_job_metadata_path(job.job_id) + job_dir = self.get_job_dir(job.job_id) self.ensure_job_dir_exists(job.job_id) - job.get_results().download_files(target=job_dir) - with metadata_path.open("w", encoding="utf-8") as f: - json.dump(job_metadata, f, ensure_ascii=False) + # Proactively refresh bearer token (because task in thread will not be able to do that + job_con = job.connection + self._refresh_bearer_token(connection=job_con) + + task = _JobDownloadTask( + root_url=job_con.root_url, + bearer_token=job_con.auth.bearer if isinstance(job_con.auth, BearerAuth) else None, + job_id=job.job_id, + df_idx=row.name, # TODO figure out correct index usage + download_dir=job_dir, + ) + _log.info(f"Submitting download task {task} to download thread pool") + self._download_pool.submit_task(task) def on_job_error(self, job: BatchJob, row): """ diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 6040fade1..e5306ff7a 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -7,7 +7,9 @@ from abc import ABC, abstractmethod from dataclasses import dataclass, field from typing import Any, Dict, List, Optional, Tuple, Union +from pathlib import Path +import json import urllib3.util import openeo @@ -140,6 +142,50 @@ def execute(self) -> _TaskResult: stats_update={"start_job error": 1}, ) +@dataclass(frozen=True) +class _JobDownloadTask(ConnectedTask): + """ + Task for downloading job results and metadata. + + :param download_dir: + Root directory where job results and metadata will be downloaded. + """ + download_dir: Path + + def execute(self) -> _TaskResult: + """ + Download job results and metadata. + """ + try: + job = self.get_connection(retry=True).job(self.job_id) + + # Ensure download directory exists + self.download_dir.mkdir(parents=True, exist_ok=True) + + # Download results + job.get_results().download_files(target=self.download_dir) + + # Download metadata + job_metadata = job.describe() + metadata_path = self.download_dir / f"job_{self.job_id}.json" + with metadata_path.open("w", encoding="utf-8") as f: + json.dump(job_metadata, f, ensure_ascii=False) + + _log.info(f"Job {self.job_id!r} results downloaded successfully") + return _TaskResult( + job_id=self.job_id, + df_idx=self.df_idx, + db_update={}, #TODO consider db updates + stats_update={"job download": 1}, + ) + except Exception as e: + _log.error(f"Failed to download results for job {self.job_id!r}: {e!r}") + return _TaskResult( + job_id=self.job_id, + df_idx=self.df_idx, + db_update={}, + stats_update={"job download error": 1}, + ) class _JobManagerWorkerThreadPool: """ From d22ac209858adf0c0f90d9dce5903abec3841730 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 28 Nov 2025 11:03:47 +0100 Subject: [PATCH 02/35] include initial unit testing --- openeo/extra/job_management/_manager.py | 5 +- openeo/extra/job_management/_thread_worker.py | 10 ++- .../job_management/test_thread_worker.py | 64 +++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index d0458ba83..c575c98e6 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -674,7 +674,7 @@ def _refresh_bearer_token(self, connection: Connection, *, max_age: float = 60) else: _log.warning("Failed to proactively refresh bearer token") - def _process_task_results( + def _process_threadworker_updates( self, worker_pool: _JobManagerWorkerThreadPool, *, @@ -756,6 +756,9 @@ def on_job_done(self, job: BatchJob, row): download_dir=job_dir, ) _log.info(f"Submitting download task {task} to download thread pool") + + if self._download_pool is None: + self._download_pool = _JobManagerWorkerThreadPool() self._download_pool.submit_task(task) def on_job_error(self, job: BatchJob, row): diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index e5306ff7a..523cce0d1 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -142,7 +142,6 @@ def execute(self) -> _TaskResult: stats_update={"start_job error": 1}, ) -@dataclass(frozen=True) class _JobDownloadTask(ConnectedTask): """ Task for downloading job results and metadata. @@ -150,12 +149,11 @@ class _JobDownloadTask(ConnectedTask): :param download_dir: Root directory where job results and metadata will be downloaded. """ - download_dir: Path + def __init__(self, download_dir: Path, **kwargs): + super().__init__(**kwargs) + object.__setattr__(self, 'download_dir', download_dir) def execute(self) -> _TaskResult: - """ - Download job results and metadata. - """ try: job = self.get_connection(retry=True).job(self.job_id) @@ -186,7 +184,7 @@ def execute(self) -> _TaskResult: db_update={}, stats_update={"job download error": 1}, ) - + class _JobManagerWorkerThreadPool: """ Thread pool-based worker that manages the execution of asynchronous tasks. diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 52ee833f1..128741ac5 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -11,6 +11,7 @@ _JobManagerWorkerThreadPool, _JobStartTask, _TaskResult, + _JobDownloadTask ) from openeo.rest._testing import DummyBackend @@ -288,3 +289,66 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): assert caplog.messages == [ "Failed to start job 'job-000': OpenEoApiError('[500] Internal: No job starting for you, buddy')" ] + + def test_download_task_in_pool(self, worker_pool, tmp_path): + # Test that download tasks can be submitted to the thread pool + # without needing actual backend functionality + task = _JobDownloadTask( + job_id="pool-job-123", + df_idx=42, + root_url="https://example.com", + bearer_token="test-token", + download_dir=tmp_path + ) + + worker_pool.submit_task(task) + results, remaining = worker_pool.process_futures(timeout=1) + + # We can't test the actual download result without a backend, + # but we can verify the task was processed + assert len(results) == 1 + result = results[0] + assert result.job_id == "pool-job-123" + assert result.df_idx == 42 + assert remaining == 0 + +class TestJobDownloadTask: + def test_download_success(self, tmp_path, caplog): + caplog.set_level(logging.INFO) + + # Test the basic functionality without complex backend setup + download_dir = tmp_path / "downloads" + task = _JobDownloadTask( + job_id="test-job-123", + df_idx=0, + root_url="https://example.com", + bearer_token="test-token", + download_dir=download_dir + ) + + # Since we can't test actual downloads without a real backend, + # we'll test that the task is properly constructed and the directory is handled + assert task.job_id == "test-job-123" + assert task.df_idx == 0 + assert task.root_url == "https://example.com" + assert task.download_dir == download_dir + # Token should be hidden in repr + assert "test-token" not in repr(task) + + def test_download_failure_handling(self, tmp_path, caplog): + caplog.set_level(logging.ERROR) + + # Test that the task properly handles execution context + # We can't easily test actual download failures without complex setup, + # but we can verify the task structure and error handling approach + download_dir = tmp_path / "downloads" + task = _JobDownloadTask( + job_id="failing-job", + df_idx=1, + root_url="https://example.com", + bearer_token="test-token", + download_dir=download_dir + ) + + # The task should be properly constructed for error handling + assert task.job_id == "failing-job" \ No newline at end of file From 7acc04304dc4829fdebd5ae44df6e483e499fe28 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Wed, 10 Dec 2025 16:19:08 +0100 Subject: [PATCH 03/35] updated unit tests --- .../job_management/test_thread_worker.py | 159 ++++++++++++------ 1 file changed, 107 insertions(+), 52 deletions(-) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 128741ac5..c28c1c3a5 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -2,7 +2,9 @@ import threading import time from dataclasses import dataclass -from typing import Iterator +from typing import Iterator, Dict, Any +from pathlib import Path +import json import pytest @@ -290,65 +292,118 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): "Failed to start job 'job-000': OpenEoApiError('[500] Internal: No job starting for you, buddy')" ] - def test_download_task_in_pool(self, worker_pool, tmp_path): - # Test that download tasks can be submitted to the thread pool - # without needing actual backend functionality - task = _JobDownloadTask( - job_id="pool-job-123", - df_idx=42, - root_url="https://example.com", - bearer_token="test-token", - download_dir=tmp_path - ) + + +import tempfile +from requests_mock import Mocker +OPENEO_BACKEND = "https://openeo.dummy.test/" + +class TestJobDownloadTask: + + # Use a temporary directory for safe file handling + @pytest.fixture + def temp_dir(self): + with tempfile.TemporaryDirectory() as temp_dir: + yield Path(temp_dir) + + def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): + """ + Test a successful job download and verify file content and stats update. + """ + job_id = "job-007" + df_idx = 42 - worker_pool.submit_task(task) - results, remaining = worker_pool.process_futures(timeout=1) + # Setup Dummy Backend + backend = DummyBackend.at_url(OPENEO_BACKEND, requests_mock=requests_mock) + backend.next_result = b"The downloaded file content." - # We can't test the actual download result without a backend, - # but we can verify the task was processed - assert len(results) == 1 - result = results[0] - assert result.job_id == "pool-job-123" - assert result.df_idx == 42 - assert remaining == 0 + # Pre-set job status to "finished" so the download link is available + backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} + + # Need to ensure job status is "finished" for download attempt to occur + backend._set_job_status(job_id=job_id, status="finished") + backend.batch_jobs[job_id]["status"] = "finished" -class TestJobDownloadTask: - def test_download_success(self, tmp_path, caplog): - caplog.set_level(logging.INFO) + download_dir = temp_dir / job_id / "results" + download_dir.mkdir(parents=True) - # Test the basic functionality without complex backend setup - download_dir = tmp_path / "downloads" + # Create the task instance task = _JobDownloadTask( - job_id="test-job-123", - df_idx=0, - root_url="https://example.com", - bearer_token="test-token", - download_dir=download_dir + root_url=OPENEO_BACKEND, + bearer_token="dummy-token-7", + job_id=job_id, + df_idx=df_idx, + download_dir=download_dir, ) + + # Execute the task + result = task.execute() + + # 4. Assertions + + # A. Verify TaskResult structure + assert isinstance(result, _TaskResult) + assert result.job_id == job_id + assert result.df_idx == df_idx + + # B. Verify stats update for the MultiBackendJobManager + assert result.stats_update == {"job download": 1} + + # C. Verify download content (crucial part of the unit test) + downloaded_file = download_dir / "result.data" + assert downloaded_file.exists() + assert downloaded_file.read_bytes() == b"The downloaded file content." + + # Verify backend interaction + get_results_calls = [c for c in requests_mock.request_history if c.method == "GET" and f"/jobs/{job_id}/results" in c.url] + assert len(get_results_calls) >= 1 + get_asset_calls = [c for c in requests_mock.request_history if c.method == "GET" and f"/jobs/{job_id}/results/result.data" in c.url] + assert len(get_asset_calls) == 1 + + def test_job_download_failure(self, requests_mock: Mocker, temp_dir: Path): + """ + Test a failed download (e.g., bad connection) and verify error reporting. + """ + job_id = "job-008" + df_idx = 55 + + # Need to ensure job status is "finished" for download attempt to occur + backend = DummyBackend.at_url(OPENEO_BACKEND, requests_mock=requests_mock) + + requests_mock.get( + f"{OPENEO_BACKEND}jobs/{job_id}/results", + status_code=500, + json={"code": "InternalError", "message": "Failed to list results"}) + + backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} - # Since we can't test actual downloads without a real backend, - # we'll test that the task is properly constructed and the directory is handled - assert task.job_id == "test-job-123" - assert task.df_idx == 0 - assert task.root_url == "https://example.com" - assert task.download_dir == download_dir - # Token should be hidden in repr - assert "test-token" not in repr(task) - - def test_download_failure_handling(self, tmp_path, caplog): - caplog.set_level(logging.ERROR) + # Need to ensure job status is "finished" for download attempt to occur + backend._set_job_status(job_id=job_id, status="finished") + backend.batch_jobs[job_id]["status"] = "finished" - # Test that the task properly handles execution context - # We can't easily test actual download failures without complex setup, - # but we can verify the task structure and error handling approach - download_dir = tmp_path / "downloads" + download_dir = temp_dir / job_id / "results" + download_dir.mkdir(parents=True) + + # Create the task instance task = _JobDownloadTask( - job_id="failing-job", - df_idx=1, - root_url="https://example.com", - bearer_token="test-token", - download_dir=download_dir + root_url=OPENEO_BACKEND, + bearer_token="dummy-token-8", + job_id=job_id, + df_idx=df_idx, + download_dir=download_dir, ) + + # Execute the task + result = task.execute() + + # Verify TaskResult structure + assert isinstance(result, _TaskResult) + assert result.job_id == job_id + assert result.df_idx == df_idx + + # Verify stats update for the MultiBackendJobManager + assert result.stats_update == {"job download error": 1} - # The task should be properly constructed for error handling - assert task.job_id == "failing-job" \ No newline at end of file + # Verify no file was created (or only empty/failed files) + assert not any(p.is_file() for p in download_dir.glob("*")) + From 246ac2f8f8fde90c629b9233263a706b7d9d0c90 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 09:54:18 +0100 Subject: [PATCH 04/35] including two simple unit tests and unifying pool usage --- .../job_management/test_thread_worker.py | 216 ++++++++---------- 1 file changed, 100 insertions(+), 116 deletions(-) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index c28c1c3a5..47bde093f 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -2,9 +2,10 @@ import threading import time from dataclasses import dataclass -from typing import Iterator, Dict, Any +from typing import Iterator from pathlib import Path -import json +import tempfile +from requests_mock import Mocker import pytest @@ -82,6 +83,103 @@ def test_hide_token(self, serializer): assert "job-123" in serialized assert secret not in serialized +class TestJobDownloadTask: + + # Use a temporary directory for safe file handling + @pytest.fixture + def temp_dir(self): + with tempfile.TemporaryDirectory() as temp_dir: + yield Path(temp_dir) + + def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): + """ + Test a successful job download and verify file content and stats update. + """ + job_id = "job-007" + df_idx = 42 + + # We set up a dummy backend to simulate the job results and assert the expected calls are triggered + backend = DummyBackend.at_url("https://openeo.dummy.test/", requests_mock=requests_mock) + backend.next_result = b"The downloaded file content." + backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} + + backend._set_job_status(job_id=job_id, status="finished") + backend.batch_jobs[job_id]["status"] = "finished" + + download_dir = temp_dir / job_id / "results" + download_dir.mkdir(parents=True) + + # Create the task instance + task = _JobDownloadTask( + root_url="https://openeo.dummy.test/", + bearer_token="dummy-token-7", + job_id=job_id, + df_idx=df_idx, + download_dir=download_dir, + ) + + # Execute the task + result = task.execute() + + # Verify TaskResult structure + assert isinstance(result, _TaskResult) + assert result.job_id == job_id + assert result.df_idx == df_idx + + # Verify stats update for the MultiBackendJobManager + assert result.stats_update == {"job download": 1} + + # Verify download content (crucial part of the unit test) + downloaded_file = download_dir / "result.data" + assert downloaded_file.exists() + assert downloaded_file.read_bytes() == b"The downloaded file content." + + + def test_job_download_failure(self, requests_mock: Mocker, temp_dir: Path): + """ + Test a failed download (e.g., bad connection) and verify error reporting. + """ + job_id = "job-008" + df_idx = 55 + + # Set up dummy backend to simulate failure during results listing + backend = DummyBackend.at_url("https://openeo.dummy.test/", requests_mock=requests_mock) + + #simulate and error when downloading the results + requests_mock.get( + f"https://openeo.dummy.test/jobs/{job_id}/results", + status_code=500, + json={"code": "InternalError", "message": "Failed to list results"}) + + backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} + backend._set_job_status(job_id=job_id, status="finished") + backend.batch_jobs[job_id]["finished"] = "error" + + download_dir = temp_dir / job_id / "results" + download_dir.mkdir(parents=True) + + # Create the task instance + task = _JobDownloadTask( + root_url="https://openeo.dummy.test/", + bearer_token="dummy-token-8", + job_id=job_id, + df_idx=df_idx, + download_dir=download_dir, + ) + + # Execute the task + result = task.execute() + + # Verify TaskResult structure + assert isinstance(result, _TaskResult) + assert result.job_id == job_id + assert result.df_idx == df_idx + + # Verify stats update for the MultiBackendJobManager + assert result.stats_update == {"job download error": 1} + + # Verify no file was created (or only empty/failed files) + assert not any(p.is_file() for p in download_dir.glob("*")) class NopTask(Task): """Do Nothing""" @@ -293,117 +391,3 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): ] - -import tempfile -from requests_mock import Mocker -OPENEO_BACKEND = "https://openeo.dummy.test/" - -class TestJobDownloadTask: - - # Use a temporary directory for safe file handling - @pytest.fixture - def temp_dir(self): - with tempfile.TemporaryDirectory() as temp_dir: - yield Path(temp_dir) - - def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): - """ - Test a successful job download and verify file content and stats update. - """ - job_id = "job-007" - df_idx = 42 - - # Setup Dummy Backend - backend = DummyBackend.at_url(OPENEO_BACKEND, requests_mock=requests_mock) - backend.next_result = b"The downloaded file content." - - # Pre-set job status to "finished" so the download link is available - backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} - - # Need to ensure job status is "finished" for download attempt to occur - backend._set_job_status(job_id=job_id, status="finished") - backend.batch_jobs[job_id]["status"] = "finished" - - download_dir = temp_dir / job_id / "results" - download_dir.mkdir(parents=True) - - # Create the task instance - task = _JobDownloadTask( - root_url=OPENEO_BACKEND, - bearer_token="dummy-token-7", - job_id=job_id, - df_idx=df_idx, - download_dir=download_dir, - ) - - # Execute the task - result = task.execute() - - # 4. Assertions - - # A. Verify TaskResult structure - assert isinstance(result, _TaskResult) - assert result.job_id == job_id - assert result.df_idx == df_idx - - # B. Verify stats update for the MultiBackendJobManager - assert result.stats_update == {"job download": 1} - - # C. Verify download content (crucial part of the unit test) - downloaded_file = download_dir / "result.data" - assert downloaded_file.exists() - assert downloaded_file.read_bytes() == b"The downloaded file content." - - # Verify backend interaction - get_results_calls = [c for c in requests_mock.request_history if c.method == "GET" and f"/jobs/{job_id}/results" in c.url] - assert len(get_results_calls) >= 1 - get_asset_calls = [c for c in requests_mock.request_history if c.method == "GET" and f"/jobs/{job_id}/results/result.data" in c.url] - assert len(get_asset_calls) == 1 - - def test_job_download_failure(self, requests_mock: Mocker, temp_dir: Path): - """ - Test a failed download (e.g., bad connection) and verify error reporting. - """ - job_id = "job-008" - df_idx = 55 - - # Need to ensure job status is "finished" for download attempt to occur - backend = DummyBackend.at_url(OPENEO_BACKEND, requests_mock=requests_mock) - - requests_mock.get( - f"{OPENEO_BACKEND}jobs/{job_id}/results", - status_code=500, - json={"code": "InternalError", "message": "Failed to list results"}) - - backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "created"} - - # Need to ensure job status is "finished" for download attempt to occur - backend._set_job_status(job_id=job_id, status="finished") - backend.batch_jobs[job_id]["status"] = "finished" - - download_dir = temp_dir / job_id / "results" - download_dir.mkdir(parents=True) - - # Create the task instance - task = _JobDownloadTask( - root_url=OPENEO_BACKEND, - bearer_token="dummy-token-8", - job_id=job_id, - df_idx=df_idx, - download_dir=download_dir, - ) - - # Execute the task - result = task.execute() - - # Verify TaskResult structure - assert isinstance(result, _TaskResult) - assert result.job_id == job_id - assert result.df_idx == df_idx - - # Verify stats update for the MultiBackendJobManager - assert result.stats_update == {"job download error": 1} - - # Verify no file was created (or only empty/failed files) - assert not any(p.is_file() for p in download_dir.glob("*")) - From d67fdd67ccad386b20333f17a9df950da751242d Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 09:58:32 +0100 Subject: [PATCH 05/35] changes to job manager --- openeo/extra/job_management/_manager.py | 1 + openeo/extra/job_management/_thread_worker.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index c575c98e6..6bff9aa1a 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -811,6 +811,7 @@ def _cancel_prolonged_job(self, job: BatchJob, row): except Exception as e: _log.error(f"Unexpected error while handling job {job.job_id}: {e}") + #TODO pull this functionality away from the manager to a general utility class? job dir creation could be reused for tje Jobdownload task def get_job_dir(self, job_id: str) -> Path: """Path to directory where job metadata, results and error logs are be saved.""" return self._root_dir / f"job_{job_id}" diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 523cce0d1..1a2c33ca9 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -173,7 +173,7 @@ def execute(self) -> _TaskResult: return _TaskResult( job_id=self.job_id, df_idx=self.df_idx, - db_update={}, #TODO consider db updates + db_update={}, #TODO consider db updates? stats_update={"job download": 1}, ) except Exception as e: From 2973beedef01432920ed16f77826d2906d8c8ca7 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 10:28:18 +0100 Subject: [PATCH 06/35] adding easy callback to check number of pending tasks on thread workers; this can be used to guarantee the download gets finished --- openeo/extra/job_management/_manager.py | 25 ++++++++++++++++++- openeo/extra/job_management/_thread_worker.py | 4 +++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 6bff9aa1a..43d43fd9c 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -400,6 +400,14 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): self._download_pool.shutdown() self._download_pool = None + if self._download_pool is not None: + # Wait for downloads to complete before shutting down + _log.info("Waiting for download tasks to complete before stopping...") + while self._download_pool.num_pending_tasks() > 0: + time.sleep(0.5) + self._download_pool.shutdown() + self._download_pool = None + if self._thread is not None: self._stop_thread = True if timeout_seconds is _UNSET: @@ -513,6 +521,8 @@ def run_jobs( ).values() ) > 0 + + or self._worker_pool.num_pending_tasks() > 0 ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) stats["run_jobs loop"] += 1 @@ -523,7 +533,20 @@ def run_jobs( stats["sleep"] += 1 # TODO; run post process after shutdown once more to ensure completion? - self.stop_job_thread() + # Wait for all download tasks to complete + if self._download_results and self._download_pool is not None: + _log.info("Waiting for download tasks to complete...") + while self._download_pool.num_pending_tasks() > 0: + self._process_threadworker_updates( + worker_pool=self._download_pool, + job_db=job_db, + stats=stats + ) + time.sleep(1) # Brief pause to avoid busy waiting + _log.info("All download tasks completed.") + + self._worker_pool.shutdown() + self._download_pool.shutdown() return stats diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 1a2c33ca9..ba00b8828 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -251,6 +251,10 @@ def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskRe self._future_task_pairs = to_keep return results, len(to_keep) + + def num_pending_tasks(self) -> int: + """Return the number of tasks that are still pending (not completed).""" + return len(self._future_task_pairs) def shutdown(self) -> None: """Shuts down the thread pool gracefully.""" From 0e7c4f5de22719cf6db9dc516f773abe60ca6e62 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 12:30:37 +0100 Subject: [PATCH 07/35] process updates through job update loop --- openeo/extra/job_management/_manager.py | 28 +++++++++---------------- 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 43d43fd9c..ee692a06c 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -511,18 +511,21 @@ def run_jobs( stats = collections.defaultdict(int) self._worker_pool = _JobManagerWorkerThreadPool() - self._download_pool = _JobManagerWorkerThreadPool() + + if self._download_results: + self._download_pool = _JobManagerWorkerThreadPool() while ( sum( job_db.count_by_status( statuses=["not_started", "created", "queued_for_start", "queued", "running"] - ).values() - ) - > 0 + ).values()) > 0 + + or (self._worker_pool is not None and self._worker_pool.num_pending_tasks() > 0) - or self._worker_pool.num_pending_tasks() > 0 + or (self._download_pool is not None and self._download_pool.num_pending_tasks() > 0) + ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) stats["run_jobs loop"] += 1 @@ -532,18 +535,6 @@ def run_jobs( time.sleep(self.poll_sleep) stats["sleep"] += 1 - # TODO; run post process after shutdown once more to ensure completion? - # Wait for all download tasks to complete - if self._download_results and self._download_pool is not None: - _log.info("Waiting for download tasks to complete...") - while self._download_pool.num_pending_tasks() > 0: - self._process_threadworker_updates( - worker_pool=self._download_pool, - job_db=job_db, - stats=stats - ) - time.sleep(1) # Brief pause to avoid busy waiting - _log.info("All download tasks completed.") self._worker_pool.shutdown() self._download_pool.shutdown() @@ -775,13 +766,14 @@ def on_job_done(self, job: BatchJob, row): root_url=job_con.root_url, bearer_token=job_con.auth.bearer if isinstance(job_con.auth, BearerAuth) else None, job_id=job.job_id, - df_idx=row.name, # TODO figure out correct index usage + df_idx=row.name, #this is going to be the index in the not saterted dataframe; should not be an issue as there is no db update for download task download_dir=job_dir, ) _log.info(f"Submitting download task {task} to download thread pool") if self._download_pool is None: self._download_pool = _JobManagerWorkerThreadPool() + self._download_pool.submit_task(task) def on_job_error(self, job: BatchJob, row): From 8ccb442a65a572ddb7301fdc38dc83d2850373a3 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 12:38:57 +0100 Subject: [PATCH 08/35] remove folder creation logic from thread to resprect optional downloa'; similarly download shutdown depend on optional download --- openeo/extra/job_management/_manager.py | 7 +++++-- openeo/extra/job_management/_thread_worker.py | 3 --- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index ee692a06c..d6676d198 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -536,8 +536,11 @@ def run_jobs( stats["sleep"] += 1 - self._worker_pool.shutdown() - self._download_pool.shutdown() + if self._worker_pool is not None: + self._worker_pool.shutdown() + + if self._download_pool is not None: + self._download_pool.shutdown() return stats diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index ba00b8828..2a55c2599 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -157,9 +157,6 @@ def execute(self) -> _TaskResult: try: job = self.get_connection(retry=True).job(self.job_id) - # Ensure download directory exists - self.download_dir.mkdir(parents=True, exist_ok=True) - # Download results job.get_results().download_files(target=self.download_dir) From 855a393aff15aa2872b3b78a7b0f36c2e4bcd6b6 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 12:39:58 +0100 Subject: [PATCH 09/35] fix stop_job_thread --- openeo/extra/job_management/_manager.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index d6676d198..e99ebd250 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -395,16 +395,12 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): if self._worker_pool is not None: self._worker_pool.shutdown() self._worker_pool = None - - if self._download_pool is not None: - self._download_pool.shutdown() - self._download_pool = None if self._download_pool is not None: # Wait for downloads to complete before shutting down _log.info("Waiting for download tasks to complete before stopping...") while self._download_pool.num_pending_tasks() > 0: - time.sleep(0.5) + time.sleep(1) self._download_pool.shutdown() self._download_pool = None From e2b6ab8d7573b2d0ed08e498ab7a366a2e41317c Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 12:52:32 +0100 Subject: [PATCH 10/35] working on fix for indefinete loop --- openeo/extra/job_management/_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index e99ebd250..9787fd678 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -534,9 +534,11 @@ def run_jobs( if self._worker_pool is not None: self._worker_pool.shutdown() + self._worker_pool = None if self._download_pool is not None: self._download_pool.shutdown() + self._download_pool = None return stats @@ -751,7 +753,6 @@ def on_job_done(self, job: BatchJob, row): :param job: The job that has finished. :param row: DataFrame row containing the job's metadata. """ - # TODO: param `row` is never accessed in this method. Remove it? Is this intended for future use? if self._download_results: job_dir = self.get_job_dir(job.job_id) From dc75ca8ec27d902ffbc18507a079a60229ea3c16 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Thu, 11 Dec 2025 13:31:04 +0100 Subject: [PATCH 11/35] fix infinite loop --- openeo/extra/job_management/_manager.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 9787fd678..c83a2a8d6 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -397,10 +397,6 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): self._worker_pool = None if self._download_pool is not None: - # Wait for downloads to complete before shutting down - _log.info("Waiting for download tasks to complete before stopping...") - while self._download_pool.num_pending_tasks() > 0: - time.sleep(1) self._download_pool.shutdown() self._download_pool = None From 4fc299de157ecf4b7485391448bcf890e673dd45 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Mon, 15 Dec 2025 08:42:21 +0100 Subject: [PATCH 12/35] wrapper to abstract multiple threadpools --- openeo/extra/job_management/_manager.py | 39 +++++---------- openeo/extra/job_management/_thread_worker.py | 48 +++++++++++++++++-- 2 files changed, 57 insertions(+), 30 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index c83a2a8d6..2c5d98674 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -221,7 +221,6 @@ def __init__( ) self._thread = None self._worker_pool = None - self._download_pool = None # Generic cache self._cache = {} @@ -354,7 +353,6 @@ def start_job_thread(self, start_job: Callable[[], BatchJob], job_db: JobDatabas self._stop_thread = False self._worker_pool = _JobManagerWorkerThreadPool() - self._download_pool = _JobManagerWorkerThreadPool() def run_loop(): # TODO: support user-provided `stats` @@ -367,6 +365,9 @@ def run_loop(): ).values() ) > 0 + + or (self._worker_pool.num_pending_tasks() > 0) + and not self._stop_thread ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) @@ -392,13 +393,10 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): .. versionadded:: 0.32.0 """ - if self._worker_pool is not None: + if self._worker_pool is not None: #TODO or thread_pool.num_pending_tasks() > 0 self._worker_pool.shutdown() self._worker_pool = None - if self._download_pool is not None: - self._download_pool.shutdown() - self._download_pool = None if self._thread is not None: self._stop_thread = True @@ -504,9 +502,6 @@ def run_jobs( self._worker_pool = _JobManagerWorkerThreadPool() - if self._download_results: - self._download_pool = _JobManagerWorkerThreadPool() - while ( sum( @@ -514,9 +509,7 @@ def run_jobs( statuses=["not_started", "created", "queued_for_start", "queued", "running"] ).values()) > 0 - or (self._worker_pool is not None and self._worker_pool.num_pending_tasks() > 0) - - or (self._download_pool is not None and self._download_pool.num_pending_tasks() > 0) + or (self._worker_pool.num_pending_tasks() > 0) ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) @@ -528,13 +521,9 @@ def run_jobs( stats["sleep"] += 1 - if self._worker_pool is not None: - self._worker_pool.shutdown() - self._worker_pool = None - - if self._download_pool is not None: - self._download_pool.shutdown() - self._download_pool = None + + self._worker_pool.shutdown() + self._worker_pool = None return stats @@ -579,8 +568,6 @@ def _job_update_loop( if self._worker_pool is not None: self._process_threadworker_updates(worker_pool=self._worker_pool, job_db=job_db, stats=stats) - if self._download_pool is not None: - self._process_threadworker_updates(worker_pool=self._download_pool, job_db=job_db, stats=stats) # TODO: move this back closer to the `_track_statuses` call above, once job done/error handling is also handled in threads? for job, row in jobs_done: @@ -657,7 +644,7 @@ def _launch_job(self, start_job, df, i, backend_name, stats: Optional[dict] = No df_idx=i, ) _log.info(f"Submitting task {task} to thread pool") - self._worker_pool.submit_task(task) + self._worker_pool.submit_start_task(task) stats["job_queued_for_start"] += 1 df.loc[i, "status"] = "queued_for_start" @@ -703,7 +690,7 @@ def _process_threadworker_updates( :param stats: Dictionary accumulating statistic counters """ # Retrieve completed task results immediately - results, _ = worker_pool.process_futures(timeout=0) + results, start_remaining, download_remaining = worker_pool.process_all_updates(timeout=0) # Collect update dicts updates: List[Dict[str, Any]] = [] @@ -767,10 +754,10 @@ def on_job_done(self, job: BatchJob, row): ) _log.info(f"Submitting download task {task} to download thread pool") - if self._download_pool is None: - self._download_pool = _JobManagerWorkerThreadPool() + if self._worker_pool is None: + self._worker_pool = _JobManagerWorkerThreadPool() - self._download_pool.submit_task(task) + self._worker_pool.submit_download_task(task) def on_job_error(self, job: BatchJob, row): """ diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 2a55c2599..1fe3e713c 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -3,6 +3,8 @@ """ import concurrent.futures +import threading +import queue import logging from abc import ABC, abstractmethod from dataclasses import dataclass, field @@ -148,12 +150,14 @@ class _JobDownloadTask(ConnectedTask): :param download_dir: Root directory where job results and metadata will be downloaded. + :param download_throttle: + A threading.Semaphore to limit concurrent downloads. """ - def __init__(self, download_dir: Path, **kwargs): - super().__init__(**kwargs) - object.__setattr__(self, 'download_dir', download_dir) + download_dir: Path = field(repr=False) + def execute(self) -> _TaskResult: + try: job = self.get_connection(retry=True).job(self.job_id) @@ -182,7 +186,7 @@ def execute(self) -> _TaskResult: stats_update={"job download error": 1}, ) -class _JobManagerWorkerThreadPool: +class _TaskThreadPool: """ Thread pool-based worker that manages the execution of asynchronous tasks. @@ -257,3 +261,39 @@ def shutdown(self) -> None: """Shuts down the thread pool gracefully.""" _log.info("Shutting down thread pool") self._executor.shutdown(wait=True) + + +class _JobManagerWorkerThreadPool: + """WRAPPER that hides two pools behind one interface""" + + def __init__(self, max_start_workers=2, max_download_workers=10): + # These are the TWO pools with their OWN _future_task_pairs + self._start_pool = _TaskThreadPool(max_workers=max_start_workers) + self._download_pool = _TaskThreadPool(max_workers=max_download_workers) + + def submit_start_task(self, task): + # Delegate to start pool + self._start_pool.submit_task(task) + + def submit_download_task(self, task): + # Delegate to download pool + self._download_pool.submit_task(task) + + def process_all_updates(self, timeout=0): + # Get results from BOTH pools + start_results, start_remaining = self._start_pool.process_futures(timeout) + download_results, download_remaining = self._download_pool.process_futures(timeout) + + # Combine and return + all_results = start_results + download_results + return all_results, start_remaining, download_remaining + + def num_pending_tasks(self): + # Sum of BOTH pools + return (self._start_pool.num_pending_tasks() + + self._download_pool.num_pending_tasks()) + + def shutdown(self): + # Shutdown BOTH pools + self._start_pool.shutdown() + self._download_pool.shutdown() From 382eae4ce816ac6e60717e2fcfb4006c6d4bf126 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Mon, 15 Dec 2025 16:18:46 +0100 Subject: [PATCH 13/35] coupling task type to seperate pool --- openeo/extra/job_management/_manager.py | 12 +- openeo/extra/job_management/_thread_worker.py | 110 ++++++++++++------ tests/extra/job_management/test_manager.py | 8 +- .../job_management/test_thread_worker.py | 2 +- 4 files changed, 85 insertions(+), 47 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 2c5d98674..ad4a6a2ff 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -367,7 +367,7 @@ def run_loop(): > 0 or (self._worker_pool.num_pending_tasks() > 0) - + and not self._stop_thread ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) @@ -644,7 +644,7 @@ def _launch_job(self, start_job, df, i, backend_name, stats: Optional[dict] = No df_idx=i, ) _log.info(f"Submitting task {task} to thread pool") - self._worker_pool.submit_start_task(task) + self._worker_pool.submit_task(task) stats["job_queued_for_start"] += 1 df.loc[i, "status"] = "queued_for_start" @@ -690,7 +690,7 @@ def _process_threadworker_updates( :param stats: Dictionary accumulating statistic counters """ # Retrieve completed task results immediately - results, start_remaining, download_remaining = worker_pool.process_all_updates(timeout=0) + results, _ = worker_pool.process_all_updates(timeout=0) # Collect update dicts updates: List[Dict[str, Any]] = [] @@ -746,10 +746,10 @@ def on_job_done(self, job: BatchJob, row): self._refresh_bearer_token(connection=job_con) task = _JobDownloadTask( - root_url=job_con.root_url, - bearer_token=job_con.auth.bearer if isinstance(job_con.auth, BearerAuth) else None, job_id=job.job_id, df_idx=row.name, #this is going to be the index in the not saterted dataframe; should not be an issue as there is no db update for download task + root_url=job_con.root_url, + bearer_token=job_con.auth.bearer if isinstance(job_con.auth, BearerAuth) else None, download_dir=job_dir, ) _log.info(f"Submitting download task {task} to download thread pool") @@ -757,7 +757,7 @@ def on_job_done(self, job: BatchJob, row): if self._worker_pool is None: self._worker_pool = _JobManagerWorkerThreadPool() - self._worker_pool.submit_download_task(task) + self._worker_pool.submit_task(task) def on_job_error(self, job: BatchJob, row): """ diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 1fe3e713c..bfb9814ef 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -3,8 +3,6 @@ """ import concurrent.futures -import threading -import queue import logging from abc import ABC, abstractmethod from dataclasses import dataclass, field @@ -103,7 +101,7 @@ def get_connection(self, retry: Union[urllib3.util.Retry, dict, bool, None] = No connection.authenticate_bearer_token(self.bearer_token) return connection - +@dataclass(frozen=True) class _JobStartTask(ConnectedTask): """ Task for starting an openEO batch job (the `POST /jobs//result` request). @@ -143,18 +141,16 @@ def execute(self) -> _TaskResult: db_update={"status": "start_failed"}, stats_update={"start_job error": 1}, ) - + +@dataclass(frozen=True) class _JobDownloadTask(ConnectedTask): """ Task for downloading job results and metadata. :param download_dir: Root directory where job results and metadata will be downloaded. - :param download_throttle: - A threading.Semaphore to limit concurrent downloads. """ - download_dir: Path = field(repr=False) - + download_dir: Path = field(default=None, repr=False) def execute(self) -> _TaskResult: @@ -198,9 +194,10 @@ class _TaskThreadPool: Defaults to 2. """ - def __init__(self, max_workers: int = 2): + def __init__(self, max_workers: int = 2, name: str = None): self._executor = concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) self._future_task_pairs: List[Tuple[concurrent.futures.Future, Task]] = [] + self._name = name def submit_task(self, task: Task) -> None: """ @@ -264,36 +261,77 @@ def shutdown(self) -> None: class _JobManagerWorkerThreadPool: - """WRAPPER that hides two pools behind one interface""" - - def __init__(self, max_start_workers=2, max_download_workers=10): - # These are the TWO pools with their OWN _future_task_pairs - self._start_pool = _TaskThreadPool(max_workers=max_start_workers) - self._download_pool = _TaskThreadPool(max_workers=max_download_workers) + + """ + Generic wrapper that manages multiple thread pools with a dict. + Uses task class names as pool names automatically. + """ - def submit_start_task(self, task): - # Delegate to start pool - self._start_pool.submit_task(task) + def __init__(self, pool_configs: Optional[Dict[str, int]] = None): + """ + :param pool_configs: Dict of task_class_name -> max_workers + Example: {"_JobStartTask": 1, "_JobDownloadTask": 2} + """ + self._pools: Dict[str, _TaskThreadPool] = {} + self._pool_configs = pool_configs or {} - def submit_download_task(self, task): - # Delegate to download pool - self._download_pool.submit_task(task) + def _get_pool_name_for_task(self, task: Task) -> str: + """ + Get pool name from task class name. + """ + return task.__class__.__name__ - def process_all_updates(self, timeout=0): - # Get results from BOTH pools - start_results, start_remaining = self._start_pool.process_futures(timeout) - download_results, download_remaining = self._download_pool.process_futures(timeout) + def submit_task(self, task: Task) -> None: + """ + Submit a task to a pool named after its class. + Creates pool dynamically if it doesn't exist. + """ + pool_name = self._get_pool_name_for_task(task) - # Combine and return - all_results = start_results + download_results - return all_results, start_remaining, download_remaining + if pool_name not in self._pools: + # Create pool on-demand + max_workers = self._pool_configs.get(pool_name, 1) # Default 1 worker + self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers, name=pool_name) + _log.info(f"Created pool '{pool_name}' with {max_workers} workers") + + self._pools[pool_name].submit_task(task) + + def process_all_updates(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskResult], Dict[str, int]]: + """ + Process updates from ALL pools. + Returns: (all_results, dict of remaining tasks per pool) + """ + all_results = [] + remaining_by_pool = {} + + for pool_name, pool in self._pools.items(): + results, remaining = pool.process_futures(timeout) + all_results.extend(results) + remaining_by_pool[pool_name] = remaining + + return all_results, remaining_by_pool - def num_pending_tasks(self): - # Sum of BOTH pools - return (self._start_pool.num_pending_tasks() + - self._download_pool.num_pending_tasks()) + def num_pending_tasks(self, pool_name: Optional[str] = None) -> int: + if pool_name: + pool = self._pools.get(pool_name) + return pool.num_pending_tasks() if pool else 0 + else: + return sum(pool.num_pending_tasks() for pool in self._pools.values()) + + def shutdown(self, pool_name: Optional[str] = None) -> None: + """ + Shutdown pools. + If pool_name is None, shuts down all pools. + """ + if pool_name: + if pool_name in self._pools: + self._pools[pool_name].shutdown() + del self._pools[pool_name] + else: + for pool_name, pool in list(self._pools.items()): + pool.shutdown() + del self._pools[pool_name] - def shutdown(self): - # Shutdown BOTH pools - self._start_pool.shutdown() - self._download_pool.shutdown() + def list_pools(self) -> List[str]: + """List all active pool names.""" + return list(self._pools.keys()) diff --git a/tests/extra/job_management/test_manager.py b/tests/extra/job_management/test_manager.py index 1d02afb1c..2c4162974 100644 --- a/tests/extra/job_management/test_manager.py +++ b/tests/extra/job_management/test_manager.py @@ -729,7 +729,7 @@ def get_status(job_id, current_status): assert isinstance(rfc3339.parse_datetime(filled_running_start_time), datetime.datetime) def test_process_threadworker_updates(self, tmp_path, caplog): - pool = _JobManagerWorkerThreadPool(max_workers=2) + pool = _JobManagerWorkerThreadPool() stats = collections.defaultdict(int) # Submit tasks covering all cases @@ -769,7 +769,7 @@ def test_process_threadworker_updates(self, tmp_path, caplog): assert caplog.messages == [] def test_process_threadworker_updates_unknown(self, tmp_path, caplog): - pool = _JobManagerWorkerThreadPool(max_workers=2) + pool = _JobManagerWorkerThreadPool() stats = collections.defaultdict(int) pool.submit_task(DummyResultTask("j-123", df_idx=0, db_update={"status": "queued"}, stats_update={"queued": 1})) @@ -806,7 +806,7 @@ def test_process_threadworker_updates_unknown(self, tmp_path, caplog): assert caplog.messages == [dirty_equals.IsStr(regex=".*Ignoring unknown.*indices.*4.*")] def test_no_results_leaves_db_and_stats_untouched(self, tmp_path, caplog): - pool = _JobManagerWorkerThreadPool(max_workers=2) + pool = _JobManagerWorkerThreadPool() stats = collections.defaultdict(int) df_initial = pd.DataFrame({"id": ["j-0"], "status": ["created"]}) @@ -820,7 +820,7 @@ def test_no_results_leaves_db_and_stats_untouched(self, tmp_path, caplog): assert stats == {} def test_logs_on_invalid_update(self, tmp_path, caplog): - pool = _JobManagerWorkerThreadPool(max_workers=2) + pool = _JobManagerWorkerThreadPool() stats = collections.defaultdict(int) # Malformed db_update (not a dict unpackable via **) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 47bde093f..04d810764 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -221,7 +221,7 @@ class TestJobManagerWorkerThreadPool: @pytest.fixture def worker_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: """Fixture for creating and cleaning up a worker thread pool.""" - pool = _JobManagerWorkerThreadPool(max_workers=2) + pool = _JobManagerWorkerThreadPool() yield pool pool.shutdown() From ab9914a7f21aa3b7d358c10aec909315723d5f98 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Mon, 15 Dec 2025 19:21:56 +0100 Subject: [PATCH 14/35] include unit test for dict of pools --- openeo/extra/job_management/_thread_worker.py | 4 +- .../job_management/test_thread_worker.py | 378 +++++++++++++++++- 2 files changed, 378 insertions(+), 4 deletions(-) diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index bfb9814ef..de5c606ec 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -194,7 +194,7 @@ class _TaskThreadPool: Defaults to 2. """ - def __init__(self, max_workers: int = 2, name: str = None): + def __init__(self, max_workers: int = 1, name: str = 'default'): self._executor = concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) self._future_task_pairs: List[Tuple[concurrent.futures.Future, Task]] = [] self._name = name @@ -335,3 +335,5 @@ def shutdown(self, pool_name: Optional[str] = None) -> None: def list_pools(self) -> List[str]: """List all active pool names.""" return list(self._pools.keys()) + + diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 04d810764..a52a763b7 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -11,6 +11,7 @@ from openeo.extra.job_management._thread_worker import ( Task, + _TaskThreadPool, _JobManagerWorkerThreadPool, _JobStartTask, _TaskResult, @@ -217,11 +218,11 @@ def execute(self) -> _TaskResult: return _TaskResult(job_id=self.job_id, df_idx=self.df_idx, db_update={"status": "all fine"}) -class TestJobManagerWorkerThreadPool: +class TestTaskThreadPool: @pytest.fixture - def worker_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: + def worker_pool(self) -> Iterator[_TaskThreadPool]: """Fixture for creating and cleaning up a worker thread pool.""" - pool = _JobManagerWorkerThreadPool() + pool = _TaskThreadPool() yield pool pool.shutdown() @@ -391,3 +392,374 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): ] +import pytest +import time +import threading +import logging +from typing import Iterator + +_log = logging.getLogger(__name__) + + +class TestJobManagerWorkerThreadPool: + @pytest.fixture + def thread_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: + """Fixture for creating and cleaning up a thread pool manager.""" + pool = _JobManagerWorkerThreadPool() + yield pool + pool.shutdown() + + @pytest.fixture + def configured_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: + """Fixture with pre-configured pools.""" + pool = _JobManagerWorkerThreadPool( + pool_configs={ + "NopTask": 2, + "DummyTask": 3, + "BlockingTask": 1, + } + ) + yield pool + pool.shutdown() + + def test_init_empty_config(self): + """Test initialization with empty config.""" + pool = _JobManagerWorkerThreadPool() + assert pool._pools == {} + assert pool._pool_configs == {} + pool.shutdown() + + def test_init_with_config(self): + """Test initialization with pool configurations.""" + pool = _JobManagerWorkerThreadPool({ + "NopTask": 2, + "DummyTask": 3, + }) + # Pools should NOT be created until first use + assert pool._pools == {} + assert pool._pool_configs == { + "NopTask": 2, + "DummyTask": 3, + } + pool.shutdown() + + def test_submit_task_creates_pool(self, thread_pool): + """Test that submitting a task creates a pool dynamically.""" + task = NopTask(job_id="j-1", df_idx=1) + + # No pools initially + assert thread_pool.list_pools() == [] + + # Submit task - should create pool + thread_pool.submit_task(task) + + # Pool should be created with default workers (1) + assert thread_pool.list_pools() == ["NopTask"] + assert "NopTask" in thread_pool._pools + + # Process to complete the task + results, remaining = thread_pool.process_all_updates(timeout=0.1) + assert len(results) == 1 + assert results[0].job_id == "j-1" + assert remaining == {"NopTask": 0} + + def test_submit_task_uses_config(self, configured_pool): + """Test that pool creation uses configuration.""" + task = NopTask(job_id="j-1", df_idx=1) + + # Submit task - should create pool with configured workers + configured_pool.submit_task(task) + + assert "NopTask" in configured_pool._pools + # Can't directly check max_workers, but pool should exist + assert "NopTask" in configured_pool.list_pools() + + def test_submit_multiple_task_types(self, thread_pool): + """Test submitting different task types to different pools.""" + # Submit different task types + task1 = NopTask(job_id="j-1", df_idx=1) + task2 = DummyTask(job_id="j-2", df_idx=2) + task3 = NopTask(job_id="j-3", df_idx=3) + + thread_pool.submit_task(task1) # Goes to "NopTask" pool + thread_pool.submit_task(task2) # Goes to "DummyTask" pool + thread_pool.submit_task(task3) # Goes to "NopTask" pool (existing) + + # Should have 2 pools + pools = sorted(thread_pool.list_pools()) + assert pools == ["DummyTask", "NopTask"] + + # Check pending tasks + assert thread_pool.num_pending_tasks() == 3 + assert thread_pool.num_pending_tasks("NopTask") == 2 + assert thread_pool.num_pending_tasks("DummyTask") == 1 + assert thread_pool.num_pending_tasks("NonExistent") == 0 + + def test_process_all_updates_empty(self, thread_pool): + """Test processing updates with no pools.""" + results, remaining = thread_pool.process_all_updates(timeout=0) + assert results == [] + assert remaining == {} + + def test_process_all_updates_multiple_pools(self, thread_pool): + """Test processing updates across multiple pools.""" + # Submit tasks to different pools + thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) # NopTask pool + thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) # NopTask pool + thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # DummyTask pool + + # Process updates + results, remaining = thread_pool.process_all_updates(timeout=0.1) + + # Should get 3 results + assert len(results) == 3 + # Check results by pool + nop_results = [r for r in results if r.job_id in ["j-1", "j-2"]] + dummy_results = [r for r in results if r.job_id == "j-3"] + assert len(nop_results) == 2 + assert len(dummy_results) == 1 + + # All tasks should be completed + assert remaining == {"NopTask": 0, "DummyTask": 0} + + def test_process_all_updates_partial_completion(self): + """Test processing when some tasks are still running.""" + # Use a pool with blocking tasks + pool = _JobManagerWorkerThreadPool() + + # Create a blocking task + event = threading.Event() + blocking_task = BlockingTask(job_id="j-block", df_idx=0, event=event, success=True) + + # Create a quick task + quick_task = NopTask(job_id="j-quick", df_idx=1) + + pool.submit_task(blocking_task) # BlockingTask pool + pool.submit_task(quick_task) # NopTask pool + + # Process with timeout=0 - only quick task should complete + results, remaining = pool.process_all_updates(timeout=0) + + # Only quick task completed + assert len(results) == 1 + assert results[0].job_id == "j-quick" + + # Blocking task still pending + assert remaining == {"BlockingTask": 1, "NopTask": 0} + assert pool.num_pending_tasks() == 1 + assert pool.num_pending_tasks("BlockingTask") == 1 + + # Release blocking task and process again + event.set() + results2, remaining2 = pool.process_all_updates(timeout=0.1) + + assert len(results2) == 1 + assert results2[0].job_id == "j-block" + assert remaining2 == {"BlockingTask": 0, "NopTask": 0} + + pool.shutdown() + + def test_num_pending_tasks(self, thread_pool): + """Test counting pending tasks.""" + # Initially empty + assert thread_pool.num_pending_tasks() == 0 + assert thread_pool.num_pending_tasks("NopTask") == 0 + + # Add some tasks + thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) + thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) + thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) + + # Check totals + assert thread_pool.num_pending_tasks() == 3 + assert thread_pool.num_pending_tasks("NopTask") == 2 + assert thread_pool.num_pending_tasks("DummyTask") == 1 + assert thread_pool.num_pending_tasks("NonExistentPool") == 0 + + # Process all + thread_pool.process_all_updates(timeout=0.1) + + # Should be empty + assert thread_pool.num_pending_tasks() == 0 + assert thread_pool.num_pending_tasks("NopTask") == 0 + + def test_shutdown_specific_pool(self): + """Test shutting down a specific pool.""" + # Create fresh pool for destructive test + pool = _JobManagerWorkerThreadPool() + + # Create two pools + pool.submit_task(NopTask(job_id="j-1", df_idx=1)) # NopTask pool + pool.submit_task(DummyTask(job_id="j-2", df_idx=2)) # DummyTask pool + + assert sorted(pool.list_pools()) == ["DummyTask", "NopTask"] + + # Shutdown NopTask pool only + pool.shutdown("NopTask") + + # Only DummyTask pool should remain + assert pool.list_pools() == ["DummyTask"] + + # Can't submit to shutdown pool + # Actually, it will create a new pool since we deleted it + pool.submit_task(NopTask(job_id="j-3", df_idx=3)) # Creates new NopTask pool + assert sorted(pool.list_pools()) == ["DummyTask", "NopTask"] + + pool.shutdown() + + def test_shutdown_all(self): + """Test shutting down all pools.""" + # Create fresh pool for destructive test + pool = _JobManagerWorkerThreadPool() + + # Create multiple pools + pool.submit_task(NopTask(job_id="j-1", df_idx=1)) + pool.submit_task(DummyTask(job_id="j-2", df_idx=2)) + + assert len(pool.list_pools()) == 2 + + # Shutdown all + pool.shutdown() + + # All pools should be gone + assert pool.list_pools() == [] + + # Can't submit any more tasks after shutdown + # Actually, shutdown() doesn't prevent creating new pools + # So we can test that shutdown clears existing pools + assert len(pool._pools) == 0 + + def test_custom_get_pool_name(self): + """Test custom task class to verify pool name selection.""" + + @dataclass(frozen=True) + class CustomTask(Task): + # Fields are inherited from Task: job_id, df_idx + + def execute(self) -> _TaskResult: + return _TaskResult(job_id=self.job_id, df_idx=self.df_idx) + + pool = _JobManagerWorkerThreadPool() + + # Submit custom task - must provide all required fields + task = CustomTask(job_id="j-1", df_idx=1) + pool.submit_task(task) + + # Pool should be named after class + assert pool.list_pools() == ["CustomTask"] + assert pool.num_pending_tasks() == 1 + + # Process it + results, remaining = pool.process_all_updates(timeout=0.1) + assert len(results) == 1 + assert results[0].job_id == "j-1" + + pool.shutdown() + + def test_concurrent_submissions(self, thread_pool): + """Test concurrent task submissions to same pool.""" + import concurrent.futures + + def submit_tasks(start_idx: int): + for i in range(5): + thread_pool.submit_task(NopTask(job_id=f"j-{start_idx + i}", df_idx=start_idx + i)) + + # Submit tasks from multiple threads + with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: + futures = [executor.submit(submit_tasks, i * 10) for i in range(3)] + concurrent.futures.wait(futures) + + # Should have all tasks in one pool + assert thread_pool.list_pools() == ["NopTask"] + assert thread_pool.num_pending_tasks() == 15 + + # Process them all + results, remaining = thread_pool.process_all_updates(timeout=0.5) + + assert len(results) == 15 + assert remaining == {"NopTask": 0} + + def test_pool_parallelism_with_blocking_tasks(self): + """Test that multiple workers allow parallel execution.""" + pool = _JobManagerWorkerThreadPool({ + "BlockingTask": 3, # 3 workers for blocking tasks + }) + + # Create multiple blocking tasks + events = [threading.Event() for _ in range(5)] + start_time = time.time() + + for i, event in enumerate(events): + pool.submit_task(BlockingTask( + job_id=f"j-block-{i}", + df_idx=i, + event=event, + success=True + )) + + # Initially all pending + assert pool.num_pending_tasks() == 5 + + # Release all events at once + for event in events: + event.set() + + # Process with timeout - all should complete + results, remaining = pool.process_all_updates(timeout=0.5) + + # All should complete (if pool had enough workers) + assert len(results) == 5 + assert remaining == {"BlockingTask": 0} + + # Check they all completed + for result in results: + assert result.job_id.startswith("j-block-") + + pool.shutdown() + + def test_task_with_error_handling(self, thread_pool): + """Test that task errors are properly handled in the pool.""" + # Submit a failing DummyTask (j-666 fails) + thread_pool.submit_task(DummyTask(job_id="j-666", df_idx=0)) + + # Process it + results, remaining = thread_pool.process_all_updates(timeout=0.1) + + # Should get error result + assert len(results) == 1 + result = results[0] + assert result.job_id == "j-666" + assert result.db_update == {"status": "threaded task failed"} + assert result.stats_update == {"threaded task failed": 1} + assert remaining == {"DummyTask": 0} + + def test_mixed_success_and_error_tasks(self, thread_pool): + """Test mix of successful and failing tasks.""" + # Submit mix of tasks + thread_pool.submit_task(DummyTask(job_id="j-1", df_idx=1)) # Success + thread_pool.submit_task(DummyTask(job_id="j-666", df_idx=2)) # Failure + thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # Success + + # Process all + results, remaining = thread_pool.process_all_updates(timeout=0.1) + + # Should get 3 results + assert len(results) == 3 + assert remaining == {"DummyTask": 0} + + # Check results + success_results = [r for r in results if r.job_id != "j-666"] + error_results = [r for r in results if r.job_id == "j-666"] + + assert len(success_results) == 2 + assert len(error_results) == 1 + + # Verify success results + for result in success_results: + assert result.db_update == {"status": "dummified"} + assert result.stats_update == {"dummy": 1} + + # Verify error result + error_result = error_results[0] + assert error_result.db_update == {"status": "threaded task failed"} + assert error_result.stats_update == {"threaded task failed": 1} \ No newline at end of file From 1fce77b89f7feec5cd77e14f509cfa06802e5c5e Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Tue, 16 Dec 2025 12:53:19 +0100 Subject: [PATCH 15/35] tmp_path usage and renaming --- openeo/extra/job_management/_manager.py | 2 +- openeo/extra/job_management/_thread_worker.py | 2 +- .../job_management/test_thread_worker.py | 76 ++++++------------- 3 files changed, 25 insertions(+), 55 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index ad4a6a2ff..ae0b38305 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -690,7 +690,7 @@ def _process_threadworker_updates( :param stats: Dictionary accumulating statistic counters """ # Retrieve completed task results immediately - results, _ = worker_pool.process_all_updates(timeout=0) + results, _ = worker_pool.process_futures(timeout=0) # Collect update dicts updates: List[Dict[str, Any]] = [] diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index de5c606ec..439a6879b 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -296,7 +296,7 @@ def submit_task(self, task: Task) -> None: self._pools[pool_name].submit_task(task) - def process_all_updates(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskResult], Dict[str, int]]: + def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskResult], Dict[str, int]]: """ Process updates from ALL pools. Returns: (all_results, dict of remaining tasks per pool) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index a52a763b7..13115aeb6 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -86,13 +86,8 @@ def test_hide_token(self, serializer): class TestJobDownloadTask: - # Use a temporary directory for safe file handling - @pytest.fixture - def temp_dir(self): - with tempfile.TemporaryDirectory() as temp_dir: - yield Path(temp_dir) - def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): + def test_job_download_success(self, requests_mock: Mocker, tmp_path: Path): """ Test a successful job download and verify file content and stats update. """ @@ -107,7 +102,7 @@ def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): backend._set_job_status(job_id=job_id, status="finished") backend.batch_jobs[job_id]["status"] = "finished" - download_dir = temp_dir / job_id / "results" + download_dir = tmp_path / job_id / "results" download_dir.mkdir(parents=True) # Create the task instance @@ -136,7 +131,7 @@ def test_job_download_success(self, requests_mock: Mocker, temp_dir: Path): assert downloaded_file.read_bytes() == b"The downloaded file content." - def test_job_download_failure(self, requests_mock: Mocker, temp_dir: Path): + def test_job_download_failure(self, requests_mock: Mocker, tmp_path: Path): """ Test a failed download (e.g., bad connection) and verify error reporting. """ @@ -156,7 +151,7 @@ def test_job_download_failure(self, requests_mock: Mocker, temp_dir: Path): backend._set_job_status(job_id=job_id, status="finished") backend.batch_jobs[job_id]["finished"] = "error" - download_dir = temp_dir / job_id / "results" + download_dir = tmp_path / job_id / "results" download_dir.mkdir(parents=True) # Create the task instance @@ -392,14 +387,6 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): ] -import pytest -import time -import threading -import logging -from typing import Iterator - -_log = logging.getLogger(__name__) - class TestJobManagerWorkerThreadPool: @pytest.fixture @@ -447,18 +434,17 @@ def test_submit_task_creates_pool(self, thread_pool): """Test that submitting a task creates a pool dynamically.""" task = NopTask(job_id="j-1", df_idx=1) - # No pools initially assert thread_pool.list_pools() == [] # Submit task - should create pool thread_pool.submit_task(task) - # Pool should be created with default workers (1) + # Pool should be created assert thread_pool.list_pools() == ["NopTask"] assert "NopTask" in thread_pool._pools # Process to complete the task - results, remaining = thread_pool.process_all_updates(timeout=0.1) + results, remaining = thread_pool.process_futures(timeout=0.1) assert len(results) == 1 assert results[0].job_id == "j-1" assert remaining == {"NopTask": 0} @@ -471,7 +457,6 @@ def test_submit_task_uses_config(self, configured_pool): configured_pool.submit_task(task) assert "NopTask" in configured_pool._pools - # Can't directly check max_workers, but pool should exist assert "NopTask" in configured_pool.list_pools() def test_submit_multiple_task_types(self, thread_pool): @@ -495,25 +480,23 @@ def test_submit_multiple_task_types(self, thread_pool): assert thread_pool.num_pending_tasks("DummyTask") == 1 assert thread_pool.num_pending_tasks("NonExistent") == 0 - def test_process_all_updates_empty(self, thread_pool): - """Test processing updates with no pools.""" - results, remaining = thread_pool.process_all_updates(timeout=0) + def test_process_futures_updates_empty(self, thread_pool): + """Test process futures with no pools.""" + results, remaining = thread_pool.process_futures(timeout=0) assert results == [] assert remaining == {} - def test_process_all_updates_multiple_pools(self, thread_pool): + def test_process_futures_updates_multiple_pools(self, thread_pool): """Test processing updates across multiple pools.""" # Submit tasks to different pools thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) # NopTask pool thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) # NopTask pool thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # DummyTask pool - # Process updates - results, remaining = thread_pool.process_all_updates(timeout=0.1) + results, remaining = thread_pool.process_futures(timeout=0.1) - # Should get 3 results assert len(results) == 3 - # Check results by pool + nop_results = [r for r in results if r.job_id in ["j-1", "j-2"]] dummy_results = [r for r in results if r.job_id == "j-3"] assert len(nop_results) == 2 @@ -522,7 +505,7 @@ def test_process_all_updates_multiple_pools(self, thread_pool): # All tasks should be completed assert remaining == {"NopTask": 0, "DummyTask": 0} - def test_process_all_updates_partial_completion(self): + def test_process_futures_updates_partial_completion(self): """Test processing when some tasks are still running.""" # Use a pool with blocking tasks pool = _JobManagerWorkerThreadPool() @@ -538,7 +521,7 @@ def test_process_all_updates_partial_completion(self): pool.submit_task(quick_task) # NopTask pool # Process with timeout=0 - only quick task should complete - results, remaining = pool.process_all_updates(timeout=0) + results, remaining = pool.process_futures(timeout=0) # Only quick task completed assert len(results) == 1 @@ -551,7 +534,7 @@ def test_process_all_updates_partial_completion(self): # Release blocking task and process again event.set() - results2, remaining2 = pool.process_all_updates(timeout=0.1) + results2, remaining2 = pool.process_futures(timeout=0.1) assert len(results2) == 1 assert results2[0].job_id == "j-block" @@ -577,7 +560,7 @@ def test_num_pending_tasks(self, thread_pool): assert thread_pool.num_pending_tasks("NonExistentPool") == 0 # Process all - thread_pool.process_all_updates(timeout=0.1) + thread_pool.process_futures(timeout=0.1) # Should be empty assert thread_pool.num_pending_tasks() == 0 @@ -620,28 +603,20 @@ def test_shutdown_all(self): # Shutdown all pool.shutdown() - - # All pools should be gone + assert pool.list_pools() == [] - - # Can't submit any more tasks after shutdown - # Actually, shutdown() doesn't prevent creating new pools - # So we can test that shutdown clears existing pools assert len(pool._pools) == 0 def test_custom_get_pool_name(self): """Test custom task class to verify pool name selection.""" @dataclass(frozen=True) - class CustomTask(Task): - # Fields are inherited from Task: job_id, df_idx - + class CustomTask(Task): def execute(self) -> _TaskResult: return _TaskResult(job_id=self.job_id, df_idx=self.df_idx) pool = _JobManagerWorkerThreadPool() - # Submit custom task - must provide all required fields task = CustomTask(job_id="j-1", df_idx=1) pool.submit_task(task) @@ -650,7 +625,7 @@ def execute(self) -> _TaskResult: assert pool.num_pending_tasks() == 1 # Process it - results, remaining = pool.process_all_updates(timeout=0.1) + results, _ = pool.process_futures(timeout=0.1) assert len(results) == 1 assert results[0].job_id == "j-1" @@ -674,7 +649,7 @@ def submit_tasks(start_idx: int): assert thread_pool.num_pending_tasks() == 15 # Process them all - results, remaining = thread_pool.process_all_updates(timeout=0.5) + results, remaining = thread_pool.process_futures(timeout=0.5) assert len(results) == 15 assert remaining == {"NopTask": 0} @@ -687,7 +662,6 @@ def test_pool_parallelism_with_blocking_tasks(self): # Create multiple blocking tasks events = [threading.Event() for _ in range(5)] - start_time = time.time() for i, event in enumerate(events): pool.submit_task(BlockingTask( @@ -704,14 +678,10 @@ def test_pool_parallelism_with_blocking_tasks(self): for event in events: event.set() - # Process with timeout - all should complete - results, remaining = pool.process_all_updates(timeout=0.5) - - # All should complete (if pool had enough workers) + results, remaining = pool.process_futures(timeout=0.5) assert len(results) == 5 assert remaining == {"BlockingTask": 0} - # Check they all completed for result in results: assert result.job_id.startswith("j-block-") @@ -723,7 +693,7 @@ def test_task_with_error_handling(self, thread_pool): thread_pool.submit_task(DummyTask(job_id="j-666", df_idx=0)) # Process it - results, remaining = thread_pool.process_all_updates(timeout=0.1) + results, remaining = thread_pool.process_futures(timeout=0.1) # Should get error result assert len(results) == 1 @@ -741,7 +711,7 @@ def test_mixed_success_and_error_tasks(self, thread_pool): thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # Success # Process all - results, remaining = thread_pool.process_all_updates(timeout=0.1) + results, remaining = thread_pool.process_futures(timeout=0.1) # Should get 3 results assert len(results) == 3 From 21992faa6d94ab8611976b8f6385db8281c281ff Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Tue, 16 Dec 2025 13:43:55 +0100 Subject: [PATCH 16/35] fix documentation --- openeo/extra/job_management/_thread_worker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 439a6879b..42a3ee097 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -191,7 +191,7 @@ class _TaskThreadPool: :param max_workers: Maximum number of concurrent threads to use for execution. - Defaults to 2. + Defaults to 1. """ def __init__(self, max_workers: int = 1, name: str = 'default'): From cdc374811f6f7f6fbae42f451fefafd38d63bf4c Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Fri, 12 Dec 2025 19:19:53 +0100 Subject: [PATCH 17/35] StacDummyBuilder.asset: "data" role by default and add "proj:" helper arguments --- openeo/_version.py | 2 +- openeo/testing/stac.py | 22 +++++++++++++++++----- tests/testing/test_stac.py | 28 +++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/openeo/_version.py b/openeo/_version.py index 5eb931bdc..3be3c38e1 100644 --- a/openeo/_version.py +++ b/openeo/_version.py @@ -1 +1 @@ -__version__ = "0.47.0a3" +__version__ = "0.47.0a4" diff --git a/openeo/testing/stac.py b/openeo/testing/stac.py index 2b8507af1..e64121667 100644 --- a/openeo/testing/stac.py +++ b/openeo/testing/stac.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Union +from typing import Any, Dict, List, Optional, Sequence, Union class StacDummyBuilder: @@ -117,13 +117,25 @@ def asset( cls, href: str = "https://stac.test/asset.tiff", type: str = "image/tiff; application=geotiff", - roles: Optional[List[str]] = None, + roles: Union[Sequence[str], None] = ("data",), + proj_code: Optional[str] = None, + proj_bbox: Optional[Sequence[float]] = None, + proj_shape: Optional[Sequence[int]] = None, + proj_transform: Optional[Sequence[float]] = None, **kwargs, ): - d = {"href": href, **kwargs} + d: Dict[str, Any] = {"href": href, **kwargs} if type: d["type"] = type - if roles is not None: - d["roles"] = roles + if roles: + d["roles"] = list(roles) + if proj_code: + d["proj:code"] = proj_code + if proj_bbox: + d["proj:bbox"] = list(proj_bbox) + if proj_shape: + d["proj:shape"] = list(proj_shape) + if proj_transform: + d["proj:transform"] = list(proj_transform) return d diff --git a/tests/testing/test_stac.py b/tests/testing/test_stac.py index 025e5928e..090e009d1 100644 --- a/tests/testing/test_stac.py +++ b/tests/testing/test_stac.py @@ -87,6 +87,32 @@ def test_asset_default(self): assert asset == { "href": "https://stac.test/asset.tiff", "type": "image/tiff; application=geotiff", + "roles": ["data"], + } + pystac.Asset.from_dict(asset) + + def test_asset_no_roles(self): + asset = StacDummyBuilder.asset(roles=None) + assert asset == { + "href": "https://stac.test/asset.tiff", + "type": "image/tiff; application=geotiff", + } + pystac.Asset.from_dict(asset) + + def test_asset_proj_fields(self): + asset = StacDummyBuilder.asset( + proj_code="EPSG:4326", + proj_bbox=(3, 51, 4, 52), + proj_shape=(100, 100), + proj_transform=(0.01, 0.0, 3.0, 0.0, -0.01, 52.0), + ) + assert asset == { + "href": "https://stac.test/asset.tiff", + "type": "image/tiff; application=geotiff", + "roles": ["data"], + "proj:code": "EPSG:4326", + "proj:bbox": [3, 51, 4, 52], + "proj:shape": [100, 100], + "proj:transform": [0.01, 0.0, 3.0, 0.0, -0.01, 52.0], } - # Check if the default asset validates pystac.Asset.from_dict(asset) From 97d155d34223e85539ef7263d1de561b825a091f Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Mon, 15 Dec 2025 09:48:59 +0100 Subject: [PATCH 18/35] Add 502/503/504 to the default request retry list refs: #835, #441 --- CHANGELOG.md | 2 ++ openeo/extra/job_management/_manager.py | 8 +++++++- openeo/utils/http.py | 3 +++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de07d1afd..fd7e5f4c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Internal reorganization of `openeo.extra.job_management` submodule to ease future development ([#741](https://github.com/Open-EO/openeo-python-client/issues/741)) +- `openeo.Connection`: add some more HTTP error codes to the default retry list: `502 Bad Gateway`, `503 Service Unavailable` and `504 Gateway Timeout` ([#835](https://github.com/Open-EO/openeo-python-client/issues/835)) + ### Removed diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index ae0b38305..6ea7858c9 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -231,7 +231,13 @@ def add_backend( parallel_jobs: int = 2, ): """ - Register a backend with a name and a Connection getter. + Register a backend with a name and a :py:class:`Connection` getter. + + .. note:: + For optimal throughput and responsiveness, it is recommended + to provide a :py:class:`Connection` instance without its own (blocking) retry behavior, + since the job manager will do retries in a non-blocking way, + allowing to take care of other tasks before retrying failed requests. :param name: Name of the backend. diff --git a/openeo/utils/http.py b/openeo/utils/http.py index c314e59b8..6c8e7258d 100644 --- a/openeo/utils/http.py +++ b/openeo/utils/http.py @@ -43,6 +43,9 @@ DEFAULT_RETRY_FORCELIST = frozenset( [ HTTP_429_TOO_MANY_REQUESTS, + HTTP_502_BAD_GATEWAY, + HTTP_503_SERVICE_UNAVAILABLE, + HTTP_504_GATEWAY_TIMEOUT, ] ) From 1b31b196a95aea68dff0523dd366a6ae5460b5dc Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Mon, 15 Dec 2025 15:12:23 +0100 Subject: [PATCH 19/35] fixup! Add 502/503/504 to the default request retry list --- tests/rest/test_job.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/test_job.py b/tests/rest/test_job.py index 19de98ef8..b5cd8d6c4 100644 --- a/tests/rest/test_job.py +++ b/tests/rest/test_job.py @@ -353,8 +353,8 @@ def test_start_and_wait_with_error_require_success(dummy_backend, require_succes pytest.raises(OpenEoApiPlainError, match=re.escape("[500] Internal Server Error")), [23], ), - ( # Default config with a 503 error (skipped by soft error feature of execute_batch poll loop) - None, + ( # Only retry by default on 429, but still handle a 503 error with soft error skipping feature of execute_batch poll loop + {"status_forcelist": [HTTP_429_TOO_MANY_REQUESTS]}, [httpretty.Response(status=HTTP_503_SERVICE_UNAVAILABLE, body="Service Unavailable")], contextlib.nullcontext(), [23, 12.34, 34], From fc29125db8e074c63f4cb40ba5ab2735a1a7456c Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Tue, 16 Dec 2025 09:06:28 +0100 Subject: [PATCH 20/35] Remove deprecated load_disk_collection/load_disk_data ref Open-EO/openeo-geopyspark-driver#1457 --- CHANGELOG.md | 3 ++- openeo/rest/connection.py | 20 -------------------- openeo/rest/datacube.py | 25 ------------------------- 3 files changed, 2 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd7e5f4c7..620ae2ad9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Internal reorganization of `openeo.extra.job_management` submodule to ease future development ([#741](https://github.com/Open-EO/openeo-python-client/issues/741)) - `openeo.Connection`: add some more HTTP error codes to the default retry list: `502 Bad Gateway`, `503 Service Unavailable` and `504 Gateway Timeout` ([#835](https://github.com/Open-EO/openeo-python-client/issues/835)) - ### Removed +- Remove `Connection.load_disk_collection` (wrapper for non-standard `load_disk_data` process), deprecated since version 0.25.0 (related to [Open-EO/openeo-geopyspark-driver#1457](https://github.com/Open-EO/openeo-geopyspark-driver/issues/1457)) + ### Fixed diff --git a/openeo/rest/connection.py b/openeo/rest/connection.py index 5625ef479..07ec9e10e 100644 --- a/openeo/rest/connection.py +++ b/openeo/rest/connection.py @@ -1873,26 +1873,6 @@ def service(self, service_id: str) -> Service: """ return Service(service_id, connection=self) - @deprecated( - reason="Depends on non-standard process, replace with :py:meth:`openeo.rest.connection.Connection.load_stac` where possible.", - version="0.25.0") - def load_disk_collection( - self, format: str, glob_pattern: str, options: Optional[dict] = None - ) -> DataCube: - """ - Loads image data from disk as a :py:class:`DataCube`. - - This is backed by a non-standard process ('load_disk_data'). This will eventually be replaced by standard options such as - :py:meth:`openeo.rest.connection.Connection.load_stac` or https://processes.openeo.org/#load_uploaded_files - - :param format: the file format, e.g. 'GTiff' - :param glob_pattern: a glob pattern that matches the files to load from disk - :param options: options specific to the file format - """ - return DataCube.load_disk_collection( - self, format, glob_pattern, **(options or {}) - ) - def as_curl( self, data: Union[dict, DataCube, FlatGraphableMixin], diff --git a/openeo/rest/datacube.py b/openeo/rest/datacube.py index 00d850d09..bd19bb871 100644 --- a/openeo/rest/datacube.py +++ b/openeo/rest/datacube.py @@ -307,31 +307,6 @@ def _build_load_properties_argument( return properties - @classmethod - @deprecated(reason="Depends on non-standard process, replace with :py:meth:`openeo.rest.connection.Connection.load_stac` where possible.",version="0.25.0") - def load_disk_collection(cls, connection: Connection, file_format: str, glob_pattern: str, **options) -> DataCube: - """ - Loads image data from disk as a DataCube. - This is backed by a non-standard process ('load_disk_data'). This will eventually be replaced by standard options such as - :py:meth:`openeo.rest.connection.Connection.load_stac` or https://processes.openeo.org/#load_uploaded_files - - - :param connection: The connection to use to connect with the backend. - :param file_format: the file format, e.g. 'GTiff' - :param glob_pattern: a glob pattern that matches the files to load from disk - :param options: options specific to the file format - :return: the data as a DataCube - """ - pg = PGNode( - process_id='load_disk_data', - arguments={ - 'format': file_format, - 'glob_pattern': glob_pattern, - 'options': options - } - ) - return cls(graph=pg, connection=connection) - @classmethod def load_stac( cls, From 615d821816358ae2b267900d2a4b942eb26aaa59 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Tue, 2 Dec 2025 18:35:23 +0100 Subject: [PATCH 21/35] Support UDF based spatial/temporal extents in load_collection/load_stac #831 --- CHANGELOG.md | 1 + openeo/internal/graph_building.py | 2 + openeo/rest/connection.py | 17 +++++-- openeo/rest/datacube.py | 20 ++++---- tests/rest/datacube/test_datacube.py | 65 +++++++++++++++++++++++ tests/rest/datacube/test_datacube100.py | 64 +++++++++++++++++++++++ tests/rest/test_connection.py | 68 +++++++++++++++++++++++++ 7 files changed, 223 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 620ae2ad9..7172a3a0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `MultiBackendJobManager`: add `download_results` option to enable/disable the automated download of job results once completed by the job manager ([#744](https://github.com/Open-EO/openeo-python-client/issues/744)) +- Support UDF based spatial and temporal extents in `load_collection`, `load_stac` and `filter_temporal` ([#831](https://github.com/Open-EO/openeo-python-client/pull/831)) ### Changed diff --git a/openeo/internal/graph_building.py b/openeo/internal/graph_building.py index 6f5918ea2..10b395fdd 100644 --- a/openeo/internal/graph_building.py +++ b/openeo/internal/graph_building.py @@ -95,6 +95,8 @@ def print_json( class _FromNodeMixin(abc.ABC): """Mixin for classes that want to hook into the generation of a "from_node" reference.""" + # TODO: rename this class: it's more an interface than a mixin, and "from node" might be confusing as explained below. + @abc.abstractmethod def from_node(self) -> PGNode: # TODO: "from_node" is a bit a confusing name: diff --git a/openeo/rest/connection.py b/openeo/rest/connection.py index 07ec9e10e..25f9030fb 100644 --- a/openeo/rest/connection.py +++ b/openeo/rest/connection.py @@ -35,7 +35,12 @@ import openeo from openeo.config import config_log, get_config_option from openeo.internal.documentation import openeo_process -from openeo.internal.graph_building import FlatGraphableMixin, PGNode, as_flat_graph +from openeo.internal.graph_building import ( + FlatGraphableMixin, + PGNode, + _FromNodeMixin, + as_flat_graph, +) from openeo.internal.jupyter import VisualDict, VisualList from openeo.internal.processes.builder import ProcessBuilderBase from openeo.internal.warnings import deprecated, legacy_alias @@ -1186,8 +1191,8 @@ def load_collection( self, collection_id: Union[str, Parameter], spatial_extent: Union[dict, Parameter, shapely.geometry.base.BaseGeometry, str, Path, None] = None, - temporal_extent: Union[Sequence[InputDate], Parameter, str, None] = None, - bands: Union[Iterable[str], Parameter, str, None] = None, + temporal_extent: Union[Sequence[InputDate], Parameter, str, _FromNodeMixin, None] = None, + bands: Union[Iterable[str], Parameter, str, _FromNodeMixin, None] = None, properties: Union[ Dict[str, Union[PGNode, Callable]], List[CollectionProperty], CollectionProperty, None ] = None, @@ -1287,8 +1292,10 @@ def load_result( def load_stac( self, url: str, - spatial_extent: Union[dict, Parameter, shapely.geometry.base.BaseGeometry, str, Path, None] = None, - temporal_extent: Union[Sequence[InputDate], Parameter, str, None] = None, + spatial_extent: Union[ + dict, Parameter, shapely.geometry.base.BaseGeometry, str, Path, _FromNodeMixin, None + ] = None, + temporal_extent: Union[Sequence[InputDate], Parameter, str, _FromNodeMixin, None] = None, bands: Union[Iterable[str], Parameter, str, None] = None, properties: Union[ Dict[str, Union[PGNode, Callable]], List[CollectionProperty], CollectionProperty, None diff --git a/openeo/rest/datacube.py b/openeo/rest/datacube.py index bd19bb871..238d0bd51 100644 --- a/openeo/rest/datacube.py +++ b/openeo/rest/datacube.py @@ -91,7 +91,7 @@ # Type annotation aliases -InputDate = Union[str, datetime.date, Parameter, PGNode, ProcessBuilderBase, None] +InputDate = Union[str, datetime.date, Parameter, PGNode, ProcessBuilderBase, _FromNodeMixin, None] class DataCube(_ProcessGraphAbstraction): @@ -165,8 +165,10 @@ def load_collection( cls, collection_id: Union[str, Parameter], connection: Optional[Connection] = None, - spatial_extent: Union[dict, Parameter, shapely.geometry.base.BaseGeometry, str, pathlib.Path, None] = None, - temporal_extent: Union[Sequence[InputDate], Parameter, str, None] = None, + spatial_extent: Union[ + dict, Parameter, shapely.geometry.base.BaseGeometry, str, pathlib.Path, _FromNodeMixin, None + ] = None, + temporal_extent: Union[Sequence[InputDate], Parameter, str, _FromNodeMixin, None] = None, bands: Union[Iterable[str], Parameter, str, None] = None, fetch_metadata: bool = True, properties: Union[ @@ -480,22 +482,22 @@ def _get_temporal_extent( *args, start_date: InputDate = None, end_date: InputDate = None, - extent: Union[Sequence[InputDate], Parameter, str, None] = None, - ) -> Union[List[Union[str, Parameter, PGNode, None]], Parameter]: + extent: Union[Sequence[InputDate], Parameter, str, _FromNodeMixin, None] = None, + ) -> Union[List[Union[str, Parameter, PGNode, _FromNodeMixin, None]], Parameter, _FromNodeMixin]: """Parameter aware temporal_extent normalizer""" # TODO: move this outside of DataCube class # TODO: return extent as tuple instead of list - if len(args) == 1 and isinstance(args[0], Parameter): + if len(args) == 1 and isinstance(args[0], (Parameter, _FromNodeMixin)): assert start_date is None and end_date is None and extent is None return args[0] - elif len(args) == 0 and isinstance(extent, Parameter): + elif len(args) == 0 and isinstance(extent, (Parameter, _FromNodeMixin)): assert start_date is None and end_date is None # TODO: warn about unexpected parameter schema return extent else: def convertor(d: Any) -> Any: # TODO: can this be generalized through _FromNodeMixin? - if isinstance(d, Parameter) or isinstance(d, PGNode): + if isinstance(d, Parameter) or isinstance(d, _FromNodeMixin): # TODO: warn about unexpected parameter schema return d elif isinstance(d, ProcessBuilderBase): @@ -531,7 +533,7 @@ def filter_temporal( *args, start_date: InputDate = None, end_date: InputDate = None, - extent: Union[Sequence[InputDate], Parameter, str, None] = None, + extent: Union[Sequence[InputDate], Parameter, str, _FromNodeMixin, None] = None, ) -> DataCube: """ Limit the DataCube to a certain date range, which can be specified in several ways: diff --git a/tests/rest/datacube/test_datacube.py b/tests/rest/datacube/test_datacube.py index f99f24726..4389d52ce 100644 --- a/tests/rest/datacube/test_datacube.py +++ b/tests/rest/datacube/test_datacube.py @@ -18,8 +18,10 @@ import shapely import shapely.geometry +import openeo.processes from openeo import collection_property from openeo.api.process import Parameter +from openeo.internal.graph_building import PGNode from openeo.metadata import SpatialDimension from openeo.rest import BandMathException, OpenEoClientException from openeo.rest._testing import build_capabilities @@ -698,6 +700,69 @@ def test_filter_temporal_single_arg(s2cube: DataCube, arg, expect_failure): _ = s2cube.filter_temporal(arg) +@pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], +) +def test_filter_temporal_from_udf(s2cube: DataCube, udf_factory): + temporal_extent = udf_factory(data=[1, 2, 3], udf="print('hello time')", runtime="Python") + cube = s2cube.filter_temporal(temporal_extent) + assert get_download_graph(cube, drop_save_result=True) == { + "loadcollection1": { + "process_id": "load_collection", + "arguments": {"id": "S2", "spatial_extent": None, "temporal_extent": None}, + }, + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello time')", "runtime": "Python"}, + }, + "filtertemporal1": { + "process_id": "filter_temporal", + "arguments": { + "data": {"from_node": "loadcollection1"}, + "extent": {"from_node": "runudf1"}, + }, + }, + } + + +@pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], +) +def test_filter_temporal_start_end_from_udf(s2cube: DataCube, udf_factory): + start = udf_factory(data=[1, 2, 3], udf="print('hello start')", runtime="Python") + end = udf_factory(data=[4, 5, 6], udf="print('hello end')", runtime="Python") + cube = s2cube.filter_temporal(start_date=start, end_date=end) + assert get_download_graph(cube, drop_save_result=True) == { + "loadcollection1": { + "process_id": "load_collection", + "arguments": {"id": "S2", "spatial_extent": None, "temporal_extent": None}, + }, + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello start')", "runtime": "Python"}, + }, + "runudf2": { + "process_id": "run_udf", + "arguments": {"data": [4, 5, 6], "udf": "print('hello end')", "runtime": "Python"}, + }, + "filtertemporal1": { + "process_id": "filter_temporal", + "arguments": { + "data": {"from_node": "loadcollection1"}, + "extent": [{"from_node": "runudf1"}, {"from_node": "runudf2"}], + }, + }, + } + + def test_max_time(s2cube, api_version): im = s2cube.max_time() graph = _get_leaf_node(im, force_flat=True) diff --git a/tests/rest/datacube/test_datacube100.py b/tests/rest/datacube/test_datacube100.py index 97801365b..2788c6a7a 100644 --- a/tests/rest/datacube/test_datacube100.py +++ b/tests/rest/datacube/test_datacube100.py @@ -2375,6 +2375,70 @@ def test_load_collection_parameterized_extents(con100, spatial_extent, temporal_ } +@pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], +) +def test_load_collection_extents_from_udf(con100, udf_factory): + spatial_extent = udf_factory(data=[1, 2, 3], udf="print('hello space')", runtime="Python") + temporal_extent = udf_factory(data=[4, 5, 6], udf="print('hello time')", runtime="Python") + cube = con100.load_collection("S2", spatial_extent=spatial_extent, temporal_extent=temporal_extent) + assert get_download_graph(cube, drop_save_result=True) == { + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello space')", "runtime": "Python"}, + }, + "runudf2": { + "process_id": "run_udf", + "arguments": {"data": [4, 5, 6], "udf": "print('hello time')", "runtime": "Python"}, + }, + "loadcollection1": { + "process_id": "load_collection", + "arguments": { + "id": "S2", + "spatial_extent": {"from_node": "runudf1"}, + "temporal_extent": {"from_node": "runudf2"}, + }, + }, + } + + +@pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], +) +def test_load_collection_temporal_extent_from_udf(con100, udf_factory): + temporal_extent = [ + udf_factory(data=[1, 2, 3], udf="print('hello start')", runtime="Python"), + udf_factory(data=[4, 5, 6], udf="print('hello end')", runtime="Python"), + ] + cube = con100.load_collection("S2", temporal_extent=temporal_extent) + assert get_download_graph(cube, drop_save_result=True) == { + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello start')", "runtime": "Python"}, + }, + "runudf2": { + "process_id": "run_udf", + "arguments": {"data": [4, 5, 6], "udf": "print('hello end')", "runtime": "Python"}, + }, + "loadcollection1": { + "process_id": "load_collection", + "arguments": { + "id": "S2", + "spatial_extent": None, + "temporal_extent": [{"from_node": "runudf1"}, {"from_node": "runudf2"}], + }, + }, + } + + def test_apply_dimension_temporal_cumsum_with_target(con100, test_data): cumsum = con100.load_collection("S2").apply_dimension('cumsum', dimension="t", target_dimension="MyNewTime") actual_graph = cumsum.flat_graph() diff --git a/tests/rest/test_connection.py b/tests/rest/test_connection.py index 0fc0a3976..6cf05e62a 100644 --- a/tests/rest/test_connection.py +++ b/tests/rest/test_connection.py @@ -17,6 +17,7 @@ import shapely.geometry import openeo +import openeo.processes from openeo import BatchJob from openeo.api.process import Parameter from openeo.internal.graph_building import FlatGraphableMixin, PGNode @@ -3715,6 +3716,73 @@ def test_load_stac_spatial_extent_vector_cube(self, dummy_backend): }, } + @pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], + ) + def test_load_stac_extents_from_udf(self, dummy_backend, udf_factory): + spatial_extent = udf_factory(data=[1, 2, 3], udf="print('hello space')", runtime="Python") + temporal_extent = udf_factory(data=[4, 5, 6], udf="print('hello time')", runtime="Python") + cube = dummy_backend.connection.load_stac( + "https://stac.test/data", spatial_extent=spatial_extent, temporal_extent=temporal_extent + ) + cube.execute() + assert dummy_backend.get_sync_pg() == { + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello space')", "runtime": "Python"}, + }, + "runudf2": { + "process_id": "run_udf", + "arguments": {"data": [4, 5, 6], "udf": "print('hello time')", "runtime": "Python"}, + }, + "loadstac1": { + "process_id": "load_stac", + "arguments": { + "url": "https://stac.test/data", + "spatial_extent": {"from_node": "runudf1"}, + "temporal_extent": {"from_node": "runudf2"}, + }, + "result": True, + }, + } + + @pytest.mark.parametrize( + "udf_factory", + [ + (lambda data, udf, runtime: openeo.processes.run_udf(data=data, udf=udf, runtime=runtime)), + (lambda data, udf, runtime: PGNode(process_id="run_udf", data=data, udf=udf, runtime=runtime)), + ], + ) + def test_load_stac_temporal_extent_from_udf(self, dummy_backend, udf_factory): + temporal_extent = [ + udf_factory(data=[1, 2, 3], udf="print('hello start')", runtime="Python"), + udf_factory(data=[4, 5, 6], udf="print('hello end')", runtime="Python"), + ] + cube = dummy_backend.connection.load_stac("https://stac.test/data", temporal_extent=temporal_extent) + cube.execute() + assert dummy_backend.get_sync_pg() == { + "runudf1": { + "process_id": "run_udf", + "arguments": {"data": [1, 2, 3], "udf": "print('hello start')", "runtime": "Python"}, + }, + "runudf2": { + "process_id": "run_udf", + "arguments": {"data": [4, 5, 6], "udf": "print('hello end')", "runtime": "Python"}, + }, + "loadstac1": { + "process_id": "load_stac", + "arguments": { + "url": "https://stac.test/data", + "temporal_extent": [{"from_node": "runudf1"}, {"from_node": "runudf2"}], + }, + "result": True, + }, + } + @pytest.mark.parametrize( "data", From 5387f89ae4921ee906260f22ef623511b925435b Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 17 Dec 2025 15:46:26 +0100 Subject: [PATCH 22/35] `MultiBackendJobManager`: don't count "created" jobs as "running" Note that this behavior was present since beginning of MultiBackendJobManager (b0d100e1), but is unnecessary as the resource consumption of "created" jobs is negligible compared to queued or running jobs. --- CHANGELOG.md | 1 + openeo/extra/job_management/_manager.py | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7172a3a0a..871da08ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Internal reorganization of `openeo.extra.job_management` submodule to ease future development ([#741](https://github.com/Open-EO/openeo-python-client/issues/741)) - `openeo.Connection`: add some more HTTP error codes to the default retry list: `502 Bad Gateway`, `503 Service Unavailable` and `504 Gateway Timeout` ([#835](https://github.com/Open-EO/openeo-python-client/issues/835)) +- `MultiBackendJobManager`: don't count "created" jobs as "running" ### Removed diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 6ea7858c9..1f63ecf13 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -553,8 +553,7 @@ def _job_update_loop( not_started = job_db.get_by_status(statuses=["not_started"], max=200).copy() if len(not_started) > 0: # Check number of jobs running at each backend - # TODO: should "created" be included in here? Calling this "running" is quite misleading then. - running = job_db.get_by_status(statuses=["created", "queued", "queued_for_start", "running"]) + running = job_db.get_by_status(statuses=["queued", "queued_for_start", "running"]) stats["job_db get_by_status"] += 1 per_backend = running.groupby("backend_name").size().to_dict() _log.info(f"Running per backend: {per_backend}") From 5d224823dbbfc9b74fb971aaf31531ee2d228c55 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 17 Dec 2025 16:11:25 +0100 Subject: [PATCH 23/35] test_start_job_thread_basic: more robust against randomness of last job --- tests/extra/job_management/test_manager.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/extra/job_management/test_manager.py b/tests/extra/job_management/test_manager.py index 2c4162974..b3c18e9b1 100644 --- a/tests/extra/job_management/test_manager.py +++ b/tests/extra/job_management/test_manager.py @@ -281,7 +281,15 @@ def test_start_job_thread_basic(self, tmp_path, job_manager, job_manager_root_di ("job-2019", "finished", "foo", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), ("job-2020", "finished", "bar", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), ("job-2021", "finished", "bar", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), - ("job-2022", "finished", "foo", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), + ( + "job-2022", + "finished", + dirty_equals.IsOneOf("foo", "bar"), + "1234.5 cpu-seconds", + "34567.89 mb-seconds", + "2345 seconds", + 123, + ), ] # Check downloaded results and metadata. From e18c820da6253cf2530d1e3acc2ca64733182c89 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 17 Dec 2025 17:44:41 +0100 Subject: [PATCH 24/35] Revert 706faf and 3d3ca1 for #839/#840 --- CHANGELOG.md | 1 - openeo/extra/job_management/_manager.py | 4 +++- tests/extra/job_management/test_manager.py | 10 +--------- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 871da08ee..7172a3a0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Internal reorganization of `openeo.extra.job_management` submodule to ease future development ([#741](https://github.com/Open-EO/openeo-python-client/issues/741)) - `openeo.Connection`: add some more HTTP error codes to the default retry list: `502 Bad Gateway`, `503 Service Unavailable` and `504 Gateway Timeout` ([#835](https://github.com/Open-EO/openeo-python-client/issues/835)) -- `MultiBackendJobManager`: don't count "created" jobs as "running" ### Removed diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 1f63ecf13..134e394f9 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -553,7 +553,9 @@ def _job_update_loop( not_started = job_db.get_by_status(statuses=["not_started"], max=200).copy() if len(not_started) > 0: # Check number of jobs running at each backend - running = job_db.get_by_status(statuses=["queued", "queued_for_start", "running"]) + # TODO: should "created" be included in here? Calling this "running" is quite misleading then. + # apparently (see #839/#840) this seemingly simple change makes a lot of MultiBackendJobManager tests flaky + running = job_db.get_by_status(statuses=["created", "queued", "queued_for_start", "running"]) stats["job_db get_by_status"] += 1 per_backend = running.groupby("backend_name").size().to_dict() _log.info(f"Running per backend: {per_backend}") diff --git a/tests/extra/job_management/test_manager.py b/tests/extra/job_management/test_manager.py index b3c18e9b1..2c4162974 100644 --- a/tests/extra/job_management/test_manager.py +++ b/tests/extra/job_management/test_manager.py @@ -281,15 +281,7 @@ def test_start_job_thread_basic(self, tmp_path, job_manager, job_manager_root_di ("job-2019", "finished", "foo", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), ("job-2020", "finished", "bar", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), ("job-2021", "finished", "bar", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), - ( - "job-2022", - "finished", - dirty_equals.IsOneOf("foo", "bar"), - "1234.5 cpu-seconds", - "34567.89 mb-seconds", - "2345 seconds", - 123, - ), + ("job-2022", "finished", "foo", "1234.5 cpu-seconds", "34567.89 mb-seconds", "2345 seconds", 123), ] # Check downloaded results and metadata. From be579f02549226a5a86aaaf5efa31c9e66818f06 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay <57984106+HansVRP@users.noreply.github.com> Date: Wed, 17 Dec 2025 18:11:56 +0100 Subject: [PATCH 25/35] MultiBackendJobManager: keep "queued" under 10 for better CDSE compat (#839) refs: #839, eu-cdse/openeo-cdse-infra#859 --------- Co-authored-by: Stefaan Lippens --- CHANGELOG.md | 1 + openeo/extra/job_management/_manager.py | 23 +++++++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7172a3a0a..d2639da7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `MultiBackendJobManager`: add `download_results` option to enable/disable the automated download of job results once completed by the job manager ([#744](https://github.com/Open-EO/openeo-python-client/issues/744)) - Support UDF based spatial and temporal extents in `load_collection`, `load_stac` and `filter_temporal` ([#831](https://github.com/Open-EO/openeo-python-client/pull/831)) +- `MultiBackendJobManager`: keep number of "queued" jobs below 10 for better CDSE compatibility ([#839](https://github.com/Open-EO/openeo-python-client/pull/839), eu-cdse/openeo-cdse-infra#859) ### Changed diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index 134e394f9..fd7c73d92 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -61,6 +61,9 @@ class _Backend(NamedTuple): # Maximum number of jobs to allow in parallel on a backend parallel_jobs: int + # Maximum number of jobs to allow in queue on a backend + queueing_limit: int = 10 + @dataclasses.dataclass(frozen=True) class _ColumnProperties: @@ -254,7 +257,8 @@ def add_backend( c = connection connection = lambda: c assert callable(connection) - self.backends[name] = _Backend(get_connection=connection, parallel_jobs=parallel_jobs) + # TODO: expose queueing_limit? + self.backends[name] = _Backend(get_connection=connection, parallel_jobs=parallel_jobs, queueing_limit=10) def _get_connection(self, backend_name: str, resilient: bool = True) -> Connection: """Get a connection for the backend and optionally make it resilient (adds retry behavior) @@ -552,18 +556,21 @@ def _job_update_loop( not_started = job_db.get_by_status(statuses=["not_started"], max=200).copy() if len(not_started) > 0: - # Check number of jobs running at each backend + # Check number of jobs queued/running at each backend # TODO: should "created" be included in here? Calling this "running" is quite misleading then. # apparently (see #839/#840) this seemingly simple change makes a lot of MultiBackendJobManager tests flaky running = job_db.get_by_status(statuses=["created", "queued", "queued_for_start", "running"]) - stats["job_db get_by_status"] += 1 - per_backend = running.groupby("backend_name").size().to_dict() - _log.info(f"Running per backend: {per_backend}") + queued = running[running["status"] == "queued"] + running_per_backend = running.groupby("backend_name").size().to_dict() + queued_per_backend = queued.groupby("backend_name").size().to_dict() + _log.info(f"{running_per_backend=} {queued_per_backend=}") + total_added = 0 for backend_name in self.backends: - backend_load = per_backend.get(backend_name, 0) - if backend_load < self.backends[backend_name].parallel_jobs: - to_add = self.backends[backend_name].parallel_jobs - backend_load + queue_capacity = self.backends[backend_name].queueing_limit - queued_per_backend.get(backend_name, 0) + run_capacity = self.backends[backend_name].parallel_jobs - running_per_backend.get(backend_name, 0) + to_add = min(queue_capacity, run_capacity) + if to_add > 0: for i in not_started.index[total_added : total_added + to_add]: self._launch_job(start_job, df=not_started, i=i, backend_name=backend_name, stats=stats) stats["job launch"] += 1 From 0b4cc09d7de2911f6c7132e5280d55ec5637fbc3 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 17 Dec 2025 21:37:07 +0100 Subject: [PATCH 26/35] Release 0.47.0 --- CHANGELOG.md | 13 +++++++++++-- openeo/_version.py | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2639da7b..98686e692 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +### Changed + +### Removed + +### Fixed + + +## [0.47.0] - 2025-12-17 + +### Added + - `MultiBackendJobManager`: add `download_results` option to enable/disable the automated download of job results once completed by the job manager ([#744](https://github.com/Open-EO/openeo-python-client/issues/744)) - Support UDF based spatial and temporal extents in `load_collection`, `load_stac` and `filter_temporal` ([#831](https://github.com/Open-EO/openeo-python-client/pull/831)) - `MultiBackendJobManager`: keep number of "queued" jobs below 10 for better CDSE compatibility ([#839](https://github.com/Open-EO/openeo-python-client/pull/839), eu-cdse/openeo-cdse-infra#859) @@ -22,8 +33,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove `Connection.load_disk_collection` (wrapper for non-standard `load_disk_data` process), deprecated since version 0.25.0 (related to [Open-EO/openeo-geopyspark-driver#1457](https://github.com/Open-EO/openeo-geopyspark-driver/issues/1457)) -### Fixed - ## [0.46.0] - 2025-10-31 diff --git a/openeo/_version.py b/openeo/_version.py index 3be3c38e1..bf97bc409 100644 --- a/openeo/_version.py +++ b/openeo/_version.py @@ -1 +1 @@ -__version__ = "0.47.0a4" +__version__ = "0.47.0" From 2746cf22d6ca6d8147f62dc6960a89ed0f914333 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 17 Dec 2025 21:42:23 +0100 Subject: [PATCH 27/35] Bump version to 0.48.0a1 --- openeo/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openeo/_version.py b/openeo/_version.py index bf97bc409..f640a084f 100644 --- a/openeo/_version.py +++ b/openeo/_version.py @@ -1 +1 @@ -__version__ = "0.47.0" +__version__ = "0.48.0a1" From b88db0280e81f968a05483c0eac65395525247d4 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 09:41:29 +0100 Subject: [PATCH 28/35] keep track of number of assets --- openeo/extra/job_management/_thread_worker.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 42a3ee097..0016ca826 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -159,6 +159,10 @@ def execute(self) -> _TaskResult: # Download results job.get_results().download_files(target=self.download_dir) + + # Count assets (files to download) + assets = job.list_results().get('assets', {}) + file_count = len(assets) # Download metadata job_metadata = job.describe() @@ -171,7 +175,7 @@ def execute(self) -> _TaskResult: job_id=self.job_id, df_idx=self.df_idx, db_update={}, #TODO consider db updates? - stats_update={"job download": 1}, + stats_update={"job download": 1, "files downloaded": file_count}, ) except Exception as e: _log.error(f"Failed to download results for job {self.job_id!r}: {e!r}") @@ -179,7 +183,7 @@ def execute(self) -> _TaskResult: job_id=self.job_id, df_idx=self.df_idx, db_update={}, - stats_update={"job download error": 1}, + stats_update={"job download error": 1, "files downloaded": 0}, ) class _TaskThreadPool: From 02eb78b2a3f42e29f88620693ba4f781613e4b76 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 09:52:56 +0100 Subject: [PATCH 29/35] avoid abreviation of number --- openeo/extra/job_management/_manager.py | 6 +++--- openeo/extra/job_management/_thread_worker.py | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index fd7c73d92..b8ae34d5b 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -376,7 +376,7 @@ def run_loop(): ) > 0 - or (self._worker_pool.num_pending_tasks() > 0) + or (self._worker_pool.number_pending_tasks() > 0) and not self._stop_thread ): @@ -403,7 +403,7 @@ def stop_job_thread(self, timeout_seconds: Optional[float] = _UNSET): .. versionadded:: 0.32.0 """ - if self._worker_pool is not None: #TODO or thread_pool.num_pending_tasks() > 0 + if self._worker_pool is not None or self._worker_pool.number_pending_tasks() > 0: self._worker_pool.shutdown() self._worker_pool = None @@ -519,7 +519,7 @@ def run_jobs( statuses=["not_started", "created", "queued_for_start", "queued", "running"] ).values()) > 0 - or (self._worker_pool.num_pending_tasks() > 0) + or (self._worker_pool.number_pending_tasks() > 0) ): self._job_update_loop(job_db=job_db, start_job=start_job, stats=stats) diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 0016ca826..ac8a69724 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -254,7 +254,7 @@ def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskRe self._future_task_pairs = to_keep return results, len(to_keep) - def num_pending_tasks(self) -> int: + def number_pending_tasks(self) -> int: """Return the number of tasks that are still pending (not completed).""" return len(self._future_task_pairs) @@ -315,12 +315,12 @@ def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskRe return all_results, remaining_by_pool - def num_pending_tasks(self, pool_name: Optional[str] = None) -> int: + def number_pending_tasks(self, pool_name: Optional[str] = None) -> int: if pool_name: pool = self._pools.get(pool_name) - return pool.num_pending_tasks() if pool else 0 + return pool.number_pending_tasks() if pool else 0 else: - return sum(pool.num_pending_tasks() for pool in self._pools.values()) + return sum(pool.number_pending_tasks() for pool in self._pools.values()) def shutdown(self, pool_name: Optional[str] = None) -> None: """ From 1f027cdf3d4d6f95b2e8571710f153475800fbaa Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 09:57:31 +0100 Subject: [PATCH 30/35] do not expose number of remaining jobs --- openeo/extra/job_management/_manager.py | 2 +- openeo/extra/job_management/_thread_worker.py | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index b8ae34d5b..e2ed51790 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -704,7 +704,7 @@ def _process_threadworker_updates( :param stats: Dictionary accumulating statistic counters """ # Retrieve completed task results immediately - results, _ = worker_pool.process_futures(timeout=0) + results = worker_pool.process_futures(timeout=0) # Collect update dicts updates: List[Dict[str, Any]] = [] diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index ac8a69724..6220e7899 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -252,7 +252,7 @@ def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskRe _log.info("process_futures: %d tasks done, %d tasks remaining", len(results), len(to_keep)) self._future_task_pairs = to_keep - return results, len(to_keep) + return results def number_pending_tasks(self) -> int: """Return the number of tasks that are still pending (not completed).""" @@ -306,14 +306,12 @@ def process_futures(self, timeout: Union[float, None] = 0) -> Tuple[List[_TaskRe Returns: (all_results, dict of remaining tasks per pool) """ all_results = [] - remaining_by_pool = {} for pool_name, pool in self._pools.items(): - results, remaining = pool.process_futures(timeout) + results = pool.process_futures(timeout) all_results.extend(results) - remaining_by_pool[pool_name] = remaining - return all_results, remaining_by_pool + return all_results def number_pending_tasks(self, pool_name: Optional[str] = None) -> int: if pool_name: From 63d742c822cb0bf29fb13260fb09adc2d2d238a8 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 10:16:30 +0100 Subject: [PATCH 31/35] abstract task name in thread pool --- openeo/extra/job_management/_manager.py | 4 ++-- openeo/extra/job_management/_thread_worker.py | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/openeo/extra/job_management/_manager.py b/openeo/extra/job_management/_manager.py index e2ed51790..b4b2bc0e5 100644 --- a/openeo/extra/job_management/_manager.py +++ b/openeo/extra/job_management/_manager.py @@ -658,7 +658,7 @@ def _launch_job(self, start_job, df, i, backend_name, stats: Optional[dict] = No df_idx=i, ) _log.info(f"Submitting task {task} to thread pool") - self._worker_pool.submit_task(task) + self._worker_pool.submit_task(task=task, pool_name="job_start") stats["job_queued_for_start"] += 1 df.loc[i, "status"] = "queued_for_start" @@ -771,7 +771,7 @@ def on_job_done(self, job: BatchJob, row): if self._worker_pool is None: self._worker_pool = _JobManagerWorkerThreadPool() - self._worker_pool.submit_task(task) + self._worker_pool.submit_task(task=task, pool_name="job_download") def on_job_error(self, job: BatchJob, row): """ diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 6220e7899..7dd4e7102 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -268,7 +268,6 @@ class _JobManagerWorkerThreadPool: """ Generic wrapper that manages multiple thread pools with a dict. - Uses task class names as pool names automatically. """ def __init__(self, pool_configs: Optional[Dict[str, int]] = None): @@ -285,17 +284,18 @@ def _get_pool_name_for_task(self, task: Task) -> str: """ return task.__class__.__name__ - def submit_task(self, task: Task) -> None: + def submit_task(self, task: Task, pool_name: str = "default") -> None: """ - Submit a task to a pool named after its class. + Submit a task to a specific pool. Creates pool dynamically if it doesn't exist. - """ - pool_name = self._get_pool_name_for_task(task) + :param task: The task to execute + :param pool_name: Which pool to use (default, download, etc.) + """ if pool_name not in self._pools: # Create pool on-demand max_workers = self._pool_configs.get(pool_name, 1) # Default 1 worker - self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers, name=pool_name) + self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers) _log.info(f"Created pool '{pool_name}' with {max_workers} workers") self._pools[pool_name].submit_task(task) From fcc0fca18ce191370833231225230fef82937c84 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 10:29:19 +0100 Subject: [PATCH 32/35] not use remaing in unit test --- .../job_management/test_thread_worker.py | 76 +++++++------------ 1 file changed, 29 insertions(+), 47 deletions(-) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 13115aeb6..506c71014 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -222,31 +222,28 @@ def worker_pool(self) -> Iterator[_TaskThreadPool]: pool.shutdown() def test_no_tasks(self, worker_pool): - results, remaining = worker_pool.process_futures(timeout=10) + results = worker_pool.process_futures(timeout=10) assert results == [] - assert remaining == 0 def test_submit_and_process(self, worker_pool): worker_pool.submit_task(DummyTask(job_id="j-123", df_idx=0)) - results, remaining = worker_pool.process_futures(timeout=10) + results = worker_pool.process_futures(timeout=10) assert results == [ _TaskResult(job_id="j-123", df_idx=0, db_update={"status": "dummified"}, stats_update={"dummy": 1}), ] - assert remaining == 0 def test_submit_and_process_zero_timeout(self, worker_pool): worker_pool.submit_task(DummyTask(job_id="j-123", df_idx=0)) # Trigger context switch time.sleep(0.1) - results, remaining = worker_pool.process_futures(timeout=0) + results = worker_pool.process_futures(timeout=0) assert results == [ _TaskResult(job_id="j-123", df_idx=0, db_update={"status": "dummified"}, stats_update={"dummy": 1}), ] - assert remaining == 0 def test_submit_and_process_with_error(self, worker_pool): worker_pool.submit_task(DummyTask(job_id="j-666", df_idx=0)) - results, remaining = worker_pool.process_futures(timeout=10) + results = worker_pool.process_futures(timeout=10) assert results == [ _TaskResult( job_id="j-666", @@ -255,20 +252,18 @@ def test_submit_and_process_with_error(self, worker_pool): stats_update={"threaded task failed": 1}, ), ] - assert remaining == 0 + def test_submit_and_process_iterative(self, worker_pool): worker_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) - results, remaining = worker_pool.process_futures(timeout=1) + results = worker_pool.process_futures(timeout=1) assert results == [_TaskResult(job_id="j-1", df_idx=1)] - assert remaining == 0 # Add some more worker_pool.submit_task(NopTask(job_id="j-22", df_idx=22)) worker_pool.submit_task(NopTask(job_id="j-222", df_idx=222)) - results, remaining = worker_pool.process_futures(timeout=1) + results = worker_pool.process_futures(timeout=1) assert results == [_TaskResult(job_id="j-22", df_idx=22), _TaskResult(job_id="j-222", df_idx=222)] - assert remaining == 0 def test_submit_multiple_simple(self, worker_pool): # A bunch of dummy tasks @@ -276,7 +271,7 @@ def test_submit_multiple_simple(self, worker_pool): worker_pool.submit_task(NopTask(job_id=f"j-{j}", df_idx=j)) # Process all of them (non-zero timeout, which should be plenty of time for all of them to finish) - results, remaining = worker_pool.process_futures(timeout=1) + results = worker_pool.process_futures(timeout=1) expected = [_TaskResult(job_id=f"j-{j}", df_idx=j) for j in range(5)] assert sorted(results, key=lambda r: r.job_id) == expected @@ -297,25 +292,24 @@ def test_submit_multiple_blocking_and_failing(self, worker_pool): ) # Initial state: nothing happened yet - results, remaining = worker_pool.process_futures(timeout=0) - assert (results, remaining) == ([], n) + results = worker_pool.process_futures(timeout=0) + assert results == [] # No changes even after timeout - results, remaining = worker_pool.process_futures(timeout=0.1) - assert (results, remaining) == ([], n) + results = worker_pool.process_futures(timeout=0.1) + assert results == [] # Set one event and wait for corresponding result events[0].set() - results, remaining = worker_pool.process_futures(timeout=0.1) + results = worker_pool.process_futures(timeout=0.1) assert results == [ _TaskResult(job_id="j-0", df_idx=0, db_update={"status": "all fine"}), ] - assert remaining == n - 1 # Release all but one event for j in range(n - 1): events[j].set() - results, remaining = worker_pool.process_futures(timeout=0.1) + results = worker_pool.process_futures(timeout=0.1) assert results == [ _TaskResult(job_id="j-1", df_idx=1, db_update={"status": "all fine"}), _TaskResult(job_id="j-2", df_idx=2, db_update={"status": "all fine"}), @@ -326,22 +320,20 @@ def test_submit_multiple_blocking_and_failing(self, worker_pool): stats_update={"threaded task failed": 1}, ), ] - assert remaining == 1 # Release all events for j in range(n): events[j].set() - results, remaining = worker_pool.process_futures(timeout=0.1) + results = worker_pool.process_futures(timeout=0.1) assert results == [ _TaskResult(job_id="j-4", df_idx=4, db_update={"status": "all fine"}), ] - assert remaining == 0 def test_shutdown(self, worker_pool): # Before shutdown worker_pool.submit_task(NopTask(job_id="j-123", df_idx=0)) - results, remaining = worker_pool.process_futures(timeout=0.1) - assert (results, remaining) == ([_TaskResult(job_id="j-123", df_idx=0)], 0) + results = worker_pool.process_futures(timeout=0.1) + assert results == [_TaskResult(job_id="j-123", df_idx=0)] worker_pool.shutdown() @@ -355,7 +347,7 @@ def test_job_start_task(self, worker_pool, dummy_backend, caplog): task = _JobStartTask(job_id=job.job_id, df_idx=0, root_url=dummy_backend.connection.root_url, bearer_token=None) worker_pool.submit_task(task) - results, remaining = worker_pool.process_futures(timeout=1) + results = worker_pool.process_futures(timeout=1) assert results == [ _TaskResult( job_id="job-000", @@ -364,7 +356,6 @@ def test_job_start_task(self, worker_pool, dummy_backend, caplog): stats_update={"job start": 1}, ) ] - assert remaining == 0 assert caplog.messages == [] def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): @@ -375,13 +366,12 @@ def test_job_start_task_failure(self, worker_pool, dummy_backend, caplog): task = _JobStartTask(job_id=job.job_id, df_idx=0, root_url=dummy_backend.connection.root_url, bearer_token=None) worker_pool.submit_task(task) - results, remaining = worker_pool.process_futures(timeout=1) + results = worker_pool.process_futures(timeout=1) assert results == [ _TaskResult( job_id="job-000", df_idx=0, db_update={"status": "start_failed"}, stats_update={"start_job error": 1} ) ] - assert remaining == 0 assert caplog.messages == [ "Failed to start job 'job-000': OpenEoApiError('[500] Internal: No job starting for you, buddy')" ] @@ -444,10 +434,9 @@ def test_submit_task_creates_pool(self, thread_pool): assert "NopTask" in thread_pool._pools # Process to complete the task - results, remaining = thread_pool.process_futures(timeout=0.1) + results = thread_pool.process_futures(timeout=0.1) assert len(results) == 1 assert results[0].job_id == "j-1" - assert remaining == {"NopTask": 0} def test_submit_task_uses_config(self, configured_pool): """Test that pool creation uses configuration.""" @@ -482,9 +471,8 @@ def test_submit_multiple_task_types(self, thread_pool): def test_process_futures_updates_empty(self, thread_pool): """Test process futures with no pools.""" - results, remaining = thread_pool.process_futures(timeout=0) + results = thread_pool.process_futures(timeout=0) assert results == [] - assert remaining == {} def test_process_futures_updates_multiple_pools(self, thread_pool): """Test processing updates across multiple pools.""" @@ -493,7 +481,7 @@ def test_process_futures_updates_multiple_pools(self, thread_pool): thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) # NopTask pool thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # DummyTask pool - results, remaining = thread_pool.process_futures(timeout=0.1) + results = thread_pool.process_futures(timeout=0.1) assert len(results) == 3 @@ -521,24 +509,22 @@ def test_process_futures_updates_partial_completion(self): pool.submit_task(quick_task) # NopTask pool # Process with timeout=0 - only quick task should complete - results, remaining = pool.process_futures(timeout=0) + results = pool.process_futures(timeout=0) # Only quick task completed assert len(results) == 1 assert results[0].job_id == "j-quick" # Blocking task still pending - assert remaining == {"BlockingTask": 1, "NopTask": 0} assert pool.num_pending_tasks() == 1 assert pool.num_pending_tasks("BlockingTask") == 1 # Release blocking task and process again event.set() - results2, remaining2 = pool.process_futures(timeout=0.1) + results2 = pool.process_futures(timeout=0.1) assert len(results2) == 1 assert results2[0].job_id == "j-block" - assert remaining2 == {"BlockingTask": 0, "NopTask": 0} pool.shutdown() @@ -625,7 +611,7 @@ def execute(self) -> _TaskResult: assert pool.num_pending_tasks() == 1 # Process it - results, _ = pool.process_futures(timeout=0.1) + results = pool.process_futures(timeout=0.1) assert len(results) == 1 assert results[0].job_id == "j-1" @@ -649,10 +635,9 @@ def submit_tasks(start_idx: int): assert thread_pool.num_pending_tasks() == 15 # Process them all - results, remaining = thread_pool.process_futures(timeout=0.5) + results = thread_pool.process_futures(timeout=0.5) assert len(results) == 15 - assert remaining == {"NopTask": 0} def test_pool_parallelism_with_blocking_tasks(self): """Test that multiple workers allow parallel execution.""" @@ -678,9 +663,8 @@ def test_pool_parallelism_with_blocking_tasks(self): for event in events: event.set() - results, remaining = pool.process_futures(timeout=0.5) + results = pool.process_futures(timeout=0.5) assert len(results) == 5 - assert remaining == {"BlockingTask": 0} for result in results: assert result.job_id.startswith("j-block-") @@ -693,7 +677,7 @@ def test_task_with_error_handling(self, thread_pool): thread_pool.submit_task(DummyTask(job_id="j-666", df_idx=0)) # Process it - results, remaining = thread_pool.process_futures(timeout=0.1) + results = thread_pool.process_futures(timeout=0.1) # Should get error result assert len(results) == 1 @@ -701,7 +685,6 @@ def test_task_with_error_handling(self, thread_pool): assert result.job_id == "j-666" assert result.db_update == {"status": "threaded task failed"} assert result.stats_update == {"threaded task failed": 1} - assert remaining == {"DummyTask": 0} def test_mixed_success_and_error_tasks(self, thread_pool): """Test mix of successful and failing tasks.""" @@ -711,11 +694,10 @@ def test_mixed_success_and_error_tasks(self, thread_pool): thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # Success # Process all - results, remaining = thread_pool.process_futures(timeout=0.1) + results = thread_pool.process_futures(timeout=0.1) # Should get 3 results assert len(results) == 3 - assert remaining == {"DummyTask": 0} # Check results success_results = [r for r in results if r.job_id != "j-666"] From 4d79b87d35ec74997498b65c2e5177a08e087498 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 11:00:28 +0100 Subject: [PATCH 33/35] fix unit tests --- .../job_management/test_thread_worker.py | 83 +++++++++---------- 1 file changed, 41 insertions(+), 42 deletions(-) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 506c71014..96cadc7ca 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -123,7 +123,7 @@ def test_job_download_success(self, requests_mock: Mocker, tmp_path: Path): assert result.df_idx == df_idx # Verify stats update for the MultiBackendJobManager - assert result.stats_update == {"job download": 1} + assert result.stats_update == {'files downloaded': 1, "job download": 1} # Verify download content (crucial part of the unit test) downloaded_file = download_dir / "result.data" @@ -172,7 +172,7 @@ def test_job_download_failure(self, requests_mock: Mocker, tmp_path: Path): assert result.df_idx == df_idx # Verify stats update for the MultiBackendJobManager - assert result.stats_update == {"job download error": 1} + assert result.stats_update == {'files downloaded': 0, "job download error": 1} # Verify no file was created (or only empty/failed files) assert not any(p.is_file() for p in download_dir.glob("*")) @@ -430,8 +430,8 @@ def test_submit_task_creates_pool(self, thread_pool): thread_pool.submit_task(task) # Pool should be created - assert thread_pool.list_pools() == ["NopTask"] - assert "NopTask" in thread_pool._pools + assert thread_pool.list_pools() == ["default"] + assert "default" in thread_pool._pools # Process to complete the task results = thread_pool.process_futures(timeout=0.1) @@ -443,31 +443,33 @@ def test_submit_task_uses_config(self, configured_pool): task = NopTask(job_id="j-1", df_idx=1) # Submit task - should create pool with configured workers - configured_pool.submit_task(task) + configured_pool.submit_task(task, "NopTask") + + assert "NopTask" in configured_pool._pools assert "NopTask" in configured_pool.list_pools() + assert "DummyTask" not in configured_pool.list_pools() def test_submit_multiple_task_types(self, thread_pool): """Test submitting different task types to different pools.""" # Submit different task types task1 = NopTask(job_id="j-1", df_idx=1) task2 = DummyTask(job_id="j-2", df_idx=2) - task3 = NopTask(job_id="j-3", df_idx=3) + task3 = DummyTask(job_id="j-3", df_idx=3) thread_pool.submit_task(task1) # Goes to "NopTask" pool thread_pool.submit_task(task2) # Goes to "DummyTask" pool - thread_pool.submit_task(task3) # Goes to "NopTask" pool (existing) + thread_pool.submit_task(task3, "seperate") # Goes to "DummyTask" pool # Should have 2 pools pools = sorted(thread_pool.list_pools()) - assert pools == ["DummyTask", "NopTask"] + assert pools == ["default", "seperate"] # Check pending tasks - assert thread_pool.num_pending_tasks() == 3 - assert thread_pool.num_pending_tasks("NopTask") == 2 - assert thread_pool.num_pending_tasks("DummyTask") == 1 - assert thread_pool.num_pending_tasks("NonExistent") == 0 + assert thread_pool.number_pending_tasks() == 3 + assert thread_pool.number_pending_tasks("default") == 2 + assert thread_pool.number_pending_tasks("seperate") == 1 def test_process_futures_updates_empty(self, thread_pool): """Test process futures with no pools.""" @@ -491,8 +493,6 @@ def test_process_futures_updates_multiple_pools(self, thread_pool): assert len(dummy_results) == 1 # All tasks should be completed - assert remaining == {"NopTask": 0, "DummyTask": 0} - def test_process_futures_updates_partial_completion(self): """Test processing when some tasks are still running.""" # Use a pool with blocking tasks @@ -505,8 +505,8 @@ def test_process_futures_updates_partial_completion(self): # Create a quick task quick_task = NopTask(job_id="j-quick", df_idx=1) - pool.submit_task(blocking_task) # BlockingTask pool - pool.submit_task(quick_task) # NopTask pool + pool.submit_task(blocking_task, "blocking") # BlockingTask pool + pool.submit_task(quick_task, "quick") # NopTask pool # Process with timeout=0 - only quick task should complete results = pool.process_futures(timeout=0) @@ -516,8 +516,8 @@ def test_process_futures_updates_partial_completion(self): assert results[0].job_id == "j-quick" # Blocking task still pending - assert pool.num_pending_tasks() == 1 - assert pool.num_pending_tasks("BlockingTask") == 1 + assert pool.number_pending_tasks() == 1 + assert pool.number_pending_tasks("blocking") == 1 # Release blocking task and process again event.set() @@ -531,26 +531,25 @@ def test_process_futures_updates_partial_completion(self): def test_num_pending_tasks(self, thread_pool): """Test counting pending tasks.""" # Initially empty - assert thread_pool.num_pending_tasks() == 0 - assert thread_pool.num_pending_tasks("NopTask") == 0 + assert thread_pool.number_pending_tasks() == 0 + assert thread_pool.number_pending_tasks("default") == 0 # Add some tasks thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) - thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) + thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3), "dummy") # Check totals - assert thread_pool.num_pending_tasks() == 3 - assert thread_pool.num_pending_tasks("NopTask") == 2 - assert thread_pool.num_pending_tasks("DummyTask") == 1 - assert thread_pool.num_pending_tasks("NonExistentPool") == 0 - + assert thread_pool.number_pending_tasks() == 3 + assert thread_pool.number_pending_tasks("default") == 2 + assert thread_pool.number_pending_tasks("dummy") == 1 + # Process all thread_pool.process_futures(timeout=0.1) # Should be empty - assert thread_pool.num_pending_tasks() == 0 - assert thread_pool.num_pending_tasks("NopTask") == 0 + assert thread_pool.number_pending_tasks() == 0 + assert thread_pool.number_pending_tasks("default") == 0 def test_shutdown_specific_pool(self): """Test shutting down a specific pool.""" @@ -558,21 +557,21 @@ def test_shutdown_specific_pool(self): pool = _JobManagerWorkerThreadPool() # Create two pools - pool.submit_task(NopTask(job_id="j-1", df_idx=1)) # NopTask pool - pool.submit_task(DummyTask(job_id="j-2", df_idx=2)) # DummyTask pool + pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool + pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") # DummyTask pool - assert sorted(pool.list_pools()) == ["DummyTask", "NopTask"] + assert sorted(pool.list_pools()) == ["dummy", "notask"] # Shutdown NopTask pool only - pool.shutdown("NopTask") + pool.shutdown("notask") # Only DummyTask pool should remain - assert pool.list_pools() == ["DummyTask"] + assert pool.list_pools() == ["dummy"] # Can't submit to shutdown pool # Actually, it will create a new pool since we deleted it pool.submit_task(NopTask(job_id="j-3", df_idx=3)) # Creates new NopTask pool - assert sorted(pool.list_pools()) == ["DummyTask", "NopTask"] + assert sorted(pool.list_pools()) == [ "default", "dummy"] pool.shutdown() @@ -582,8 +581,8 @@ def test_shutdown_all(self): pool = _JobManagerWorkerThreadPool() # Create multiple pools - pool.submit_task(NopTask(job_id="j-1", df_idx=1)) - pool.submit_task(DummyTask(job_id="j-2", df_idx=2)) + pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool + pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") assert len(pool.list_pools()) == 2 @@ -604,11 +603,11 @@ def execute(self) -> _TaskResult: pool = _JobManagerWorkerThreadPool() task = CustomTask(job_id="j-1", df_idx=1) - pool.submit_task(task) + pool.submit_task(task, "custom_pool") # Pool should be named after class - assert pool.list_pools() == ["CustomTask"] - assert pool.num_pending_tasks() == 1 + assert pool.list_pools() == ["custom_pool"] + assert pool.number_pending_tasks() == 1 # Process it results = pool.process_futures(timeout=0.1) @@ -631,8 +630,8 @@ def submit_tasks(start_idx: int): concurrent.futures.wait(futures) # Should have all tasks in one pool - assert thread_pool.list_pools() == ["NopTask"] - assert thread_pool.num_pending_tasks() == 15 + assert thread_pool.list_pools() == ["default"] + assert thread_pool.number_pending_tasks() == 15 # Process them all results = thread_pool.process_futures(timeout=0.5) @@ -657,7 +656,7 @@ def test_pool_parallelism_with_blocking_tasks(self): )) # Initially all pending - assert pool.num_pending_tasks() == 5 + assert pool.number_pending_tasks() == 5 # Release all events at once for event in events: From 597997fe5a79a86539111f3b908190e38614a3ef Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 11:01:20 +0100 Subject: [PATCH 34/35] fix --- tests/extra/job_management/test_thread_worker.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index 96cadc7ca..d0d684e3b 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -4,7 +4,6 @@ from dataclasses import dataclass from typing import Iterator from pathlib import Path -import tempfile from requests_mock import Mocker import pytest From 3234f4f0b966f8e1ac01f62e522b894d40bf17e2 Mon Sep 17 00:00:00 2001 From: Hans Vanrompay Date: Fri, 19 Dec 2025 16:03:09 +0100 Subject: [PATCH 35/35] working on unit tests --- openeo/extra/job_management/_thread_worker.py | 42 ++- .../job_management/test_thread_worker.py | 343 ++++++++++++------ 2 files changed, 248 insertions(+), 137 deletions(-) diff --git a/openeo/extra/job_management/_thread_worker.py b/openeo/extra/job_management/_thread_worker.py index 7dd4e7102..86610f50f 100644 --- a/openeo/extra/job_management/_thread_worker.py +++ b/openeo/extra/job_management/_thread_worker.py @@ -202,6 +202,7 @@ def __init__(self, max_workers: int = 1, name: str = 'default'): self._executor = concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) self._future_task_pairs: List[Tuple[concurrent.futures.Future, Task]] = [] self._name = name + self._max_workers = max_workers def submit_task(self, task: Task) -> None: """ @@ -272,31 +273,34 @@ class _JobManagerWorkerThreadPool: def __init__(self, pool_configs: Optional[Dict[str, int]] = None): """ - :param pool_configs: Dict of task_class_name -> max_workers - Example: {"_JobStartTask": 1, "_JobDownloadTask": 2} + :param pool_configs: Dict of pool_name -> max_workers + Example: {"job_start": 1, "download": 2} """ self._pools: Dict[str, _TaskThreadPool] = {} self._pool_configs = pool_configs or {} - - def _get_pool_name_for_task(self, task: Task) -> str: - """ - Get pool name from task class name. - """ - return task.__class__.__name__ + + # Create all pools upfront from config + for pool_name, max_workers in self._pool_configs.items(): + self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers) + _log.info(f"Created pool '{pool_name}' with {max_workers} workers") def submit_task(self, task: Task, pool_name: str = "default") -> None: """ Submit a task to a specific pool. - Creates pool dynamically if it doesn't exist. - - :param task: The task to execute - :param pool_name: Which pool to use (default, download, etc.) + Creates pool dynamically only if not in config. """ if pool_name not in self._pools: - # Create pool on-demand - max_workers = self._pool_configs.get(pool_name, 1) # Default 1 worker - self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers) - _log.info(f"Created pool '{pool_name}' with {max_workers} workers") + # Check if pool_name is in config but somehow wasn't created + if pool_name in self._pool_configs: + # This shouldn't happen, but create it + max_workers = self._pool_configs[pool_name] + self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers, name=pool_name) + _log.warning(f"Created missing pool '{pool_name}' from config with {max_workers} workers") + else: + # Not in config - create with default + max_workers = 1 + self._pools[pool_name] = _TaskThreadPool(max_workers=max_workers, name=pool_name) + _log.info(f"Created dynamic pool '{pool_name}' with {max_workers} workers") self._pools[pool_name].submit_task(task) @@ -329,10 +333,16 @@ def shutdown(self, pool_name: Optional[str] = None) -> None: if pool_name in self._pools: self._pools[pool_name].shutdown() del self._pools[pool_name] + if pool_name in self._pool_configs: + del self._pool_configs[pool_name] else: for pool_name, pool in list(self._pools.items()): pool.shutdown() del self._pools[pool_name] + if pool_name in self._pool_configs: + del self._pool_configs[pool_name] + + self._pool_configs.clear() def list_pools(self) -> List[str]: """List all active pool names.""" diff --git a/tests/extra/job_management/test_thread_worker.py b/tests/extra/job_management/test_thread_worker.py index d0d684e3b..b1f830f77 100644 --- a/tests/extra/job_management/test_thread_worker.py +++ b/tests/extra/job_management/test_thread_worker.py @@ -129,6 +129,12 @@ def test_job_download_success(self, requests_mock: Mocker, tmp_path: Path): assert downloaded_file.exists() assert downloaded_file.read_bytes() == b"The downloaded file content." + # Verify metadata file + metadata_file = download_dir / f"job_{job_id}.json" + assert metadata_file.exists() + metadata = metadata_file.read_text() + assert job_id in metadata + def test_job_download_failure(self, requests_mock: Mocker, tmp_path: Path): """ @@ -176,6 +182,32 @@ def test_job_download_failure(self, requests_mock: Mocker, tmp_path: Path): # Verify no file was created (or only empty/failed files) assert not any(p.is_file() for p in download_dir.glob("*")) + def test_download_directory_permission_error(self, requests_mock: Mocker, tmp_path: Path): + """Test download when directory has permission issues.""" + job_id = "job-perm" + df_idx = 99 + + backend = DummyBackend.at_url("https://openeo.dummy.test/", requests_mock=requests_mock) + backend.batch_jobs[job_id] = {"job_id": job_id, "pg": {}, "status": "finished"} + + # Create a read-only directory (simulate permission issue on some systems) + download_dir = tmp_path / "readonly" + download_dir.mkdir() + + task = _JobDownloadTask( + root_url="https://openeo.dummy.test/", + bearer_token="token", + job_id=job_id, + df_idx=df_idx, + download_dir=download_dir, + ) + + # Should handle permission error gracefully + result = task.execute() + assert result.job_id == job_id + assert result.stats_update["job download error"] == 1 + + class NopTask(Task): """Do Nothing""" @@ -385,18 +417,6 @@ def thread_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: yield pool pool.shutdown() - @pytest.fixture - def configured_pool(self) -> Iterator[_JobManagerWorkerThreadPool]: - """Fixture with pre-configured pools.""" - pool = _JobManagerWorkerThreadPool( - pool_configs={ - "NopTask": 2, - "DummyTask": 3, - "BlockingTask": 1, - } - ) - yield pool - pool.shutdown() def test_init_empty_config(self): """Test initialization with empty config.""" @@ -406,18 +426,49 @@ def test_init_empty_config(self): pool.shutdown() def test_init_with_config(self): + """Test that pools are created immediately from config (not lazily).""" + pool_configs = {"default": 3, "download": 5} + pool = _JobManagerWorkerThreadPool(pool_configs) + + # Pools should exist immediately (eager initialization) + assert set(pool.list_pools()) == {"default", "download"} + assert pool._pools["default"]._max_workers == 3 + assert pool._pools["download"]._max_workers == 5 + + def test_init_with_duplicate_pool_names(self): + """Test behavior with duplicate pool names in config.""" + pool_configs = {"duplicate": 2, "duplicate": 3} + pool = _JobManagerWorkerThreadPool(pool_configs) + + # Last value should win + assert pool._pools["duplicate"]._max_workers == 3 + + + def test_submit_with_with_config(self): """Test initialization with pool configurations.""" - pool = _JobManagerWorkerThreadPool({ - "NopTask": 2, - "DummyTask": 3, - }) - # Pools should NOT be created until first use - assert pool._pools == {} - assert pool._pool_configs == { - "NopTask": 2, - "DummyTask": 3, + pool_configs = { + "default": 2, + "download": 3, + "processing": 4 } - pool.shutdown() + pool = _JobManagerWorkerThreadPool(pool_configs) + + # Pools should NOT be created until first use + # Pools should NOT be created until first use (lazy initialization) + assert len(pool._pools) == 3 # All 3 pools should exist + assert pool._pool_configs == pool_configs + assert set(pool._pools.keys()) == {"default", "download", "processing"} + + task = NopTask(job_id="j-1", df_idx=1) + pool.submit_task(task = task, pool_name = 'no_task') + + # Verify each pool has correct number of workers + assert pool._pools["default"]._max_workers == 2 + assert pool._pools["download"]._max_workers == 3 + assert pool._pools["processing"]._max_workers == 4 + assert pool._pools["no_task"]._max_workers == 1 + + def test_submit_task_creates_pool(self, thread_pool): """Test that submitting a task creates a pool dynamically.""" @@ -437,18 +488,6 @@ def test_submit_task_creates_pool(self, thread_pool): assert len(results) == 1 assert results[0].job_id == "j-1" - def test_submit_task_uses_config(self, configured_pool): - """Test that pool creation uses configuration.""" - task = NopTask(job_id="j-1", df_idx=1) - - # Submit task - should create pool with configured workers - configured_pool.submit_task(task, "NopTask") - - - - assert "NopTask" in configured_pool._pools - assert "NopTask" in configured_pool.list_pools() - assert "DummyTask" not in configured_pool.list_pools() def test_submit_multiple_task_types(self, thread_pool): """Test submitting different task types to different pools.""" @@ -457,9 +496,9 @@ def test_submit_multiple_task_types(self, thread_pool): task2 = DummyTask(job_id="j-2", df_idx=2) task3 = DummyTask(job_id="j-3", df_idx=3) - thread_pool.submit_task(task1) # Goes to "NopTask" pool - thread_pool.submit_task(task2) # Goes to "DummyTask" pool - thread_pool.submit_task(task3, "seperate") # Goes to "DummyTask" pool + thread_pool.submit_task(task1) + thread_pool.submit_task(task2) + thread_pool.submit_task(task3, "seperate") # Should have 2 pools pools = sorted(thread_pool.list_pools()) @@ -478,9 +517,9 @@ def test_process_futures_updates_empty(self, thread_pool): def test_process_futures_updates_multiple_pools(self, thread_pool): """Test processing updates across multiple pools.""" # Submit tasks to different pools - thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) # NopTask pool - thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) # NopTask pool - thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3)) # DummyTask pool + thread_pool.submit_task(NopTask(job_id="j-1", df_idx=1)) + thread_pool.submit_task(NopTask(job_id="j-2", df_idx=2)) + thread_pool.submit_task(DummyTask(job_id="j-3", df_idx=3), "dummy") results = thread_pool.process_futures(timeout=0.1) @@ -491,7 +530,6 @@ def test_process_futures_updates_multiple_pools(self, thread_pool): assert len(nop_results) == 2 assert len(dummy_results) == 1 - # All tasks should be completed def test_process_futures_updates_partial_completion(self): """Test processing when some tasks are still running.""" # Use a pool with blocking tasks @@ -527,6 +565,28 @@ def test_process_futures_updates_partial_completion(self): pool.shutdown() + def test_concurrent_submissions(self, thread_pool): + """Test concurrent task submissions to same pool.""" + import concurrent.futures + + def submit_tasks(start_idx: int): + for i in range(5): + thread_pool.submit_task(NopTask(job_id=f"j-{start_idx + i}", df_idx=start_idx + i)) + + # Submit tasks from multiple threads + with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: + futures = [executor.submit(submit_tasks, i * 10) for i in range(3)] + concurrent.futures.wait(futures) + + # Should have all tasks in one pool + assert thread_pool.list_pools() == ["default"] + assert thread_pool.number_pending_tasks() == 15 + + # Process them all + results = thread_pool.process_futures(timeout=0.5) + + assert len(results) == 15 + def test_num_pending_tasks(self, thread_pool): """Test counting pending tasks.""" # Initially empty @@ -550,92 +610,30 @@ def test_num_pending_tasks(self, thread_pool): assert thread_pool.number_pending_tasks() == 0 assert thread_pool.number_pending_tasks("default") == 0 - def test_shutdown_specific_pool(self): - """Test shutting down a specific pool.""" - # Create fresh pool for destructive test - pool = _JobManagerWorkerThreadPool() - - # Create two pools - pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool - pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") # DummyTask pool - - assert sorted(pool.list_pools()) == ["dummy", "notask"] - - # Shutdown NopTask pool only - pool.shutdown("notask") - - # Only DummyTask pool should remain - assert pool.list_pools() == ["dummy"] - - # Can't submit to shutdown pool - # Actually, it will create a new pool since we deleted it - pool.submit_task(NopTask(job_id="j-3", df_idx=3)) # Creates new NopTask pool - assert sorted(pool.list_pools()) == [ "default", "dummy"] - - pool.shutdown() - - def test_shutdown_all(self): - """Test shutting down all pools.""" - # Create fresh pool for destructive test - pool = _JobManagerWorkerThreadPool() - - # Create multiple pools - pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool - pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") - - assert len(pool.list_pools()) == 2 - - # Shutdown all - pool.shutdown() - - assert pool.list_pools() == [] - assert len(pool._pools) == 0 - def test_custom_get_pool_name(self): - """Test custom task class to verify pool name selection.""" + def test_process_futures_returns_remaining_counts(self): + """Test that process_futures returns remaining task counts per pool.""" + # Note: Your current implementation doesn't return remaining counts! + # It returns List[_TaskResult] but the docstring says it returns + # (all_results, dict of remaining tasks per pool) - @dataclass(frozen=True) - class CustomTask(Task): - def execute(self) -> _TaskResult: - return _TaskResult(job_id=self.job_id, df_idx=self.df_idx) + pool = _JobManagerWorkerThreadPool({"fast": 1, "slow": 1}) - pool = _JobManagerWorkerThreadPool() - - task = CustomTask(job_id="j-1", df_idx=1) - pool.submit_task(task, "custom_pool") + # Add blocking and non-blocking tasks + event = threading.Event() + pool.submit_task(BlockingTask(job_id="j-block", df_idx=0, event=event), "slow") + pool.submit_task(NopTask(job_id="j-quick", df_idx=1), "fast") - # Pool should be named after class - assert pool.list_pools() == ["custom_pool"] - assert pool.number_pending_tasks() == 1 + # Process with timeout=0 + results = pool.process_futures(timeout=0) - # Process it - results = pool.process_futures(timeout=0.1) + # Should get only quick task assert len(results) == 1 - assert results[0].job_id == "j-1" - - pool.shutdown() - - def test_concurrent_submissions(self, thread_pool): - """Test concurrent task submissions to same pool.""" - import concurrent.futures - - def submit_tasks(start_idx: int): - for i in range(5): - thread_pool.submit_task(NopTask(job_id=f"j-{start_idx + i}", df_idx=start_idx + i)) - - # Submit tasks from multiple threads - with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: - futures = [executor.submit(submit_tasks, i * 10) for i in range(3)] - concurrent.futures.wait(futures) - - # Should have all tasks in one pool - assert thread_pool.list_pools() == ["default"] - assert thread_pool.number_pending_tasks() == 15 - - # Process them all - results = thread_pool.process_futures(timeout=0.5) + assert results[0].job_id == "j-quick" - assert len(results) == 15 + # Check remaining tasks + assert pool.number_pending_tasks("slow") == 1 + assert pool.number_pending_tasks("fast") == 0 def test_pool_parallelism_with_blocking_tasks(self): """Test that multiple workers allow parallel execution.""" @@ -712,4 +710,107 @@ def test_mixed_success_and_error_tasks(self, thread_pool): # Verify error result error_result = error_results[0] assert error_result.db_update == {"status": "threaded task failed"} - assert error_result.stats_update == {"threaded task failed": 1} \ No newline at end of file + assert error_result.stats_update == {"threaded task failed": 1} + + def test_submit_same_task_to_multiple_pools(self): + """Test submitting the same task instance to different pools.""" + pool = _JobManagerWorkerThreadPool({"pool1": 1, "pool2": 1}) + task = NopTask(job_id="j-1", df_idx=1) + + # Submit same task to two different pools + pool.submit_task(task, "pool1") + pool.submit_task(task, "pool2") # Same instance! + + assert pool.number_pending_tasks("pool1") == 1 + assert pool.number_pending_tasks("pool2") == 1 + assert pool.number_pending_tasks() == 2 + + def test_shutdown_specific_pool(self): + """Test shutting down a specific pool.""" + # Create fresh pool for destructive test + pool = _JobManagerWorkerThreadPool() + + # Create two pools + pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool + pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") # DummyTask pool + + assert sorted(pool.list_pools()) == ["dummy", "notask"] + + # Shutdown NopTask pool only + pool.shutdown("notask") + + # Only DummyTask pool should remain + assert pool.list_pools() == ["dummy"] + + # Can't submit to shutdown pool + # Actually, it will create a new pool since we deleted it + pool.submit_task(NopTask(job_id="j-3", df_idx=3)) # Creates new NopTask pool + assert sorted(pool.list_pools()) == [ "default", "dummy"] + + pool.shutdown() + + def test_shutdown_all(self): + """Test shutting down all pools.""" + # Create fresh pool for destructive test + pool = _JobManagerWorkerThreadPool() + + # Create multiple pools + pool.submit_task(NopTask(job_id="j-1", df_idx=1), "notask") # NopTask pool + pool.submit_task(DummyTask(job_id="j-2", df_idx=2), "dummy") + + assert len(pool.list_pools()) == 2 + + # Shutdown all + pool.shutdown() + + assert pool.list_pools() == [] + assert len(pool._pools) == 0 + + + def test_submit_to_nonexistent_pool_after_shutdown(self): + """Test submitting to a pool that was previously shutdown.""" + pool = _JobManagerWorkerThreadPool({"download": 2}) + + # Shutdown the pool + pool.shutdown("download") + assert "download" not in pool.list_pools() + + # Submit again - should create new pool (since dynamic creation allowed) + task = NopTask(job_id="j-1", df_idx=1) + pool.submit_task(task, "download") + + assert "download" in pool.list_pools() + assert pool._pools["download"]._max_workers == 1 + + + def test_number_pending_tasks_for_shutdown_pool(self): + """Test checking pending tasks for a pool that was shutdown.""" + pool = _JobManagerWorkerThreadPool({"download": 2}) + + # Submit and process many tasks + for i in range(10): + pool.submit_task(NopTask(job_id=f"j-{i}", df_idx=i)) + + pool.shutdown("download") + + # Should return 0 for shutdown/nonexistent pool + assert pool.number_pending_tasks("download") == 0 + + def test_future_task_pairs_postprocessing(self): + """Test that completed tasks don't accumulate in _future_task_pairs.""" + pool = _JobManagerWorkerThreadPool() + + # Submit and process many tasks + for i in range(100): + pool.submit_task(NopTask(job_id=f"j-{i}", df_idx=i)) + + # Process all + results = pool.process_futures(timeout=0.1) + assert len(results) == 100 + + # Internal tracking list should be empty + for pool_obj in pool._pools.values(): + assert len(pool_obj._future_task_pairs) == 0 + + pool.shutdown() +