From 06156895082a2cdd31dc5835bd91662701765a98 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 7 Feb 2024 09:37:37 +0100 Subject: [PATCH 1/4] Issue #112 introduce get_backend_config for AggregatorBackendConfig --- setup.py | 2 +- src/openeo_aggregator/app.py | 1 + src/openeo_aggregator/config.py | 8 +++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/setup.py b/setup.py index 54d23b45..26bd8886 100644 --- a/setup.py +++ b/setup.py @@ -28,7 +28,7 @@ "requests", "attrs", "openeo>=0.27.0", - "openeo_driver>=0.81.0.dev", + "openeo_driver>=0.86.0.dev", "flask~=2.0", "gunicorn~=20.0", "python-json-logger>=2.0.0", diff --git a/src/openeo_aggregator/app.py b/src/openeo_aggregator/app.py index 364f1035..7449c910 100644 --- a/src/openeo_aggregator/app.py +++ b/src/openeo_aggregator/app.py @@ -39,6 +39,7 @@ def create_app(config: Any = None, auto_logging_setup: bool = True) -> flask.Fla log_version_info(logger=_log) + # TODO #112 move this default to the AggregatorBackendConfig getter (get_backend_config) os.environ.setdefault(ConfigGetter.OPENEO_BACKEND_CONFIG, str(get_config_dir() / "backend_config.py")) config: AggregatorConfig = get_config(config) diff --git a/src/openeo_aggregator/config.py b/src/openeo_aggregator/config.py index 8a654486..8b317fca 100644 --- a/src/openeo_aggregator/config.py +++ b/src/openeo_aggregator/config.py @@ -1,10 +1,11 @@ import logging import os from pathlib import Path -from typing import List, Union +from typing import Callable, List, Union import attrs from openeo_driver.config import OpenEoBackendConfig +from openeo_driver.config.load import ConfigGetter from openeo_driver.server import build_backend_deploy_metadata from openeo_driver.users.oidc import OidcProvider from openeo_driver.utils import dict_item @@ -133,3 +134,8 @@ class AggregatorBackendConfig(OpenEoBackendConfig): capabilities_deploy_metadata: dict = build_backend_deploy_metadata( packages=["openeo", "openeo_driver", "openeo_aggregator"], ) + + +_config_getter = ConfigGetter(expected_class=AggregatorBackendConfig) + +get_backend_config: Callable[..., AggregatorBackendConfig] = _config_getter.get From aae2338a28b578d2d2328a0731ae1fd6a3bd2123 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 7 Feb 2024 09:56:32 +0100 Subject: [PATCH 2/4] Issue #112 replace (test related) flask_error_handling config with create_app option --- src/openeo_aggregator/app.py | 4 ++-- src/openeo_aggregator/config.py | 1 - tests/conftest.py | 10 +++++++--- tests/test_config.py | 1 - 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/openeo_aggregator/app.py b/src/openeo_aggregator/app.py index 7449c910..5e565e73 100644 --- a/src/openeo_aggregator/app.py +++ b/src/openeo_aggregator/app.py @@ -27,7 +27,7 @@ _log = logging.getLogger(__name__) -def create_app(config: Any = None, auto_logging_setup: bool = True) -> flask.Flask: +def create_app(config: Any = None, auto_logging_setup: bool = True, flask_error_handling: bool = True) -> flask.Flask: """ Flask application factory function. """ @@ -54,7 +54,7 @@ def create_app(config: Any = None, auto_logging_setup: bool = True) -> flask.Fla _log.info(f"Building Flask app with {backend_implementation=!r}") app = openeo_driver.views.build_app( backend_implementation=backend_implementation, - error_handling=config.flask_error_handling, + error_handling=flask_error_handling, ) app.config.from_mapping( diff --git a/src/openeo_aggregator/config.py b/src/openeo_aggregator/config.py index 8b317fca..f61a9218 100644 --- a/src/openeo_aggregator/config.py +++ b/src/openeo_aggregator/config.py @@ -44,7 +44,6 @@ class AggregatorConfig(dict): # Dictionary mapping backend id to backend url aggregator_backends = dict_item() - flask_error_handling = dict_item(default=True) streaming_chunk_size = dict_item(default=STREAM_CHUNK_SIZE_DEFAULT) # TODO: add validation/normalization to make sure we have a real list of OidcProvider objects? diff --git a/tests/conftest.py b/tests/conftest.py index a1804922..7a82cece 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -177,9 +177,13 @@ def multi_backend_connection(config) -> MultiBackendConnection: def get_flask_app(config: AggregatorConfig) -> flask.Flask: - app = create_app(config=config, auto_logging_setup=False) - app.config['TESTING'] = True - app.config['SERVER_NAME'] = 'oeoa.test' + app = create_app( + config=config, + auto_logging_setup=False, + # flask_error_handling=False, # Failing test debug tip: set to False for deeper stack trace insights + ) + app.config["TESTING"] = True + app.config["SERVER_NAME"] = "oeoa.test" return app diff --git a/tests/test_config.py b/tests/test_config.py index a7d9c319..46f9eb33 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -31,7 +31,6 @@ def test_config_defaults(): config = AggregatorConfig() with pytest.raises(KeyError): _ = config.aggregator_backends - assert config.flask_error_handling is True assert config.streaming_chunk_size == STREAM_CHUNK_SIZE_DEFAULT From d1f9b752f388f7f399cfbd3dfb138cebb517f265 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 7 Feb 2024 15:44:30 +0100 Subject: [PATCH 3/4] Issue #112 port streaming_chunk_size config, introduce config_overrides for testing --- src/openeo_aggregator/backend.py | 4 ++-- src/openeo_aggregator/config.py | 8 ++++++-- src/openeo_aggregator/testing.py | 33 ++++++++++++++++++++++++++++++-- tests/test_config.py | 10 +++++----- tests/test_views.py | 7 ++++--- 5 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/openeo_aggregator/backend.py b/src/openeo_aggregator/backend.py index 453201b0..8d05260a 100644 --- a/src/openeo_aggregator/backend.py +++ b/src/openeo_aggregator/backend.py @@ -71,6 +71,7 @@ CONNECTION_TIMEOUT_JOB_START, CONNECTION_TIMEOUT_RESULT, AggregatorConfig, + get_backend_config, ) from openeo_aggregator.connection import ( BackendConnection, @@ -353,7 +354,6 @@ def __init__( self._memoizer = memoizer_from_config(config=config, namespace="Processing") self.backends.on_connections_change.add(self._memoizer.invalidate) self._catalog = catalog - self._stream_chunk_size = config.streaming_chunk_size def get_process_registry( self, api_version: Union[str, ComparableVersion] @@ -537,7 +537,7 @@ def evaluate(self, process_graph: dict, env: EvalEnv = None): _log.error(f"Failed to process synchronously on backend {con.id}: {e!r}", exc_info=True) raise OpenEOApiException(message=f"Failed to process synchronously on backend {con.id}: {e!r}") - return streaming_flask_response(backend_response, chunk_size=self._stream_chunk_size) + return streaming_flask_response(backend_response, chunk_size=get_backend_config().streaming_chunk_size) def preprocess_process_graph(self, process_graph: FlatPG, backend_id: str) -> dict: def preprocess(node: Any) -> Any: diff --git a/src/openeo_aggregator/config.py b/src/openeo_aggregator/config.py index f61a9218..415cd0ce 100644 --- a/src/openeo_aggregator/config.py +++ b/src/openeo_aggregator/config.py @@ -44,8 +44,6 @@ class AggregatorConfig(dict): # Dictionary mapping backend id to backend url aggregator_backends = dict_item() - streaming_chunk_size = dict_item(default=STREAM_CHUNK_SIZE_DEFAULT) - # TODO: add validation/normalization to make sure we have a real list of OidcProvider objects? configured_oidc_providers: List[OidcProvider] = dict_item(default=[]) auth_entitlement_check: Union[bool, dict] = dict_item(default=False) @@ -63,6 +61,9 @@ class AggregatorConfig(dict): # List of collection ids to cover with the aggregator (when None: support union of all upstream collections) collection_whitelist = dict_item(default=None) + # Just a config field for test purposes (while were stripping down this config class) + test_dummy = dict_item(default="alice") + @staticmethod def from_py_file(path: Union[str, Path]) -> 'AggregatorConfig': """Load config from Python file.""" @@ -134,6 +135,9 @@ class AggregatorBackendConfig(OpenEoBackendConfig): packages=["openeo", "openeo_driver", "openeo_aggregator"], ) + streaming_chunk_size: int = STREAM_CHUNK_SIZE_DEFAULT + + _config_getter = ConfigGetter(expected_class=AggregatorBackendConfig) diff --git a/src/openeo_aggregator/testing.py b/src/openeo_aggregator/testing.py index db134123..c9b04c74 100644 --- a/src/openeo_aggregator/testing.py +++ b/src/openeo_aggregator/testing.py @@ -1,19 +1,19 @@ -import collections import dataclasses import datetime import itertools import json import pathlib -import time from typing import Any, Dict, List, Optional, Tuple, Union from unittest import mock import kazoo import kazoo.exceptions +import openeo_driver.testing import pytest from openeo.util import rfc3339 import openeo_aggregator.about +import openeo_aggregator.config from openeo_aggregator.utils import Clock @@ -290,3 +290,32 @@ def processes(self, *args) -> dict: processes.append(process) return {"processes": processes, "links": []} + + +def config_overrides(**kwargs): + """ + *Only to be used in unit tests* + + `mock.patch` based mocker to override the config returned by `get_backend_config()` at run time + + Can be used as context manager + + >>> with config_overrides(id="foobar"): + ... ... + + in a fixture (as context manager): + + >>> @pytest.fixture + ... def custom_setup() + ... with config_overrides(id="foobar"): + ... yield + + or as test function decorator + + >>> @config_overrides(id="foobar") + ... def test_stuff(): + """ + return openeo_driver.testing.config_overrides( + config_getter=openeo_aggregator.config._config_getter, + **kwargs, + ) diff --git a/tests/test_config.py b/tests/test_config.py index 46f9eb33..acd6790a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -21,7 +21,7 @@ def _get_config_content(config_var_name: str = "config"): {config_var_name} = AggregatorConfig( config_source=__file__, aggregator_backends={{"b1": "https://b1.test"}}, - streaming_chunk_size=123 + test_dummy="bob", ) """ ) @@ -31,7 +31,7 @@ def test_config_defaults(): config = AggregatorConfig() with pytest.raises(KeyError): _ = config.aggregator_backends - assert config.streaming_chunk_size == STREAM_CHUNK_SIZE_DEFAULT + assert config.test_dummy == "alice" def test_config_aggregator_backends(): @@ -48,7 +48,7 @@ def test_config_from_py_file(tmp_path, config_var_name): config = AggregatorConfig.from_py_file(path) assert config.config_source == str(path) assert config.aggregator_backends == {"b1": "https://b1.test"} - assert config.streaming_chunk_size == 123 + assert config.test_dummy == "bob" def test_config_from_py_file_wrong_config_var_name(tmp_path): @@ -71,7 +71,7 @@ def test_get_config_py_file_path(tmp_path, convertor): config = get_config(convertor(config_path)) assert config.config_source == str(config_path) assert config.aggregator_backends == {"b1": "https://b1.test"} - assert config.streaming_chunk_size == 123 + assert config.test_dummy == "bob" def test_get_config_env_py_file(tmp_path): @@ -82,4 +82,4 @@ def test_get_config_env_py_file(tmp_path): config = get_config() assert config.config_source == str(path) assert config.aggregator_backends == {"b1": "https://b1.test"} - assert config.streaming_chunk_size == 123 + assert config.test_dummy == "bob" diff --git a/tests/test_views.py b/tests/test_views.py index 041b44b3..5618f224 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -33,7 +33,7 @@ STAC_PROPERTY_FEDERATION_BACKENDS, STAC_PROPERTY_PROVIDER_BACKEND, ) -from openeo_aggregator.testing import clock_mock +from openeo_aggregator.testing import clock_mock, config_overrides from .conftest import assert_dict_subset, get_api100, get_flask_app @@ -800,7 +800,6 @@ def test_result_basic_math_error(self, api100, requests_mock, backend1, backend2 @pytest.mark.parametrize(["chunk_size"], [(16,), (128,)]) def test_result_large_response_streaming(self, config, chunk_size, requests_mock, backend1, backend2): - config.streaming_chunk_size = chunk_size api100 = get_api100(get_flask_app(config)) def post_result(request: requests.Request, context): @@ -813,7 +812,9 @@ def post_result(request: requests.Request, context): api100.set_auth_bearer_token(token=TEST_USER_BEARER_TOKEN) pg = {"large": {"process_id": "large", "arguments": {}, "result": True}} request = {"process": {"process_graph": pg}} - res = api100.post("/result", json=request).assert_status_code(200) + + with config_overrides(streaming_chunk_size=chunk_size): + res = api100.post("/result", json=request).assert_status_code(200) assert res.response.is_streamed chunks = res.response.iter_encoded() From fb0ccee62933e5d86422da5473d9c6de3fe07ae8 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Wed, 7 Feb 2024 16:08:03 +0100 Subject: [PATCH 4/4] Bump version to 0.18.0 (#112 related) --- CHANGELOG.md | 5 +++++ src/openeo_aggregator/about.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c11d47d..ab6fd3a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. The format is roughly based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). +## [0.18.0] + +- Start porting `AggregatorConfig` fields to `AggregatorBackendConfig` ([#112](https://github.com/Open-EO/openeo-aggregator/issues/112)) + + ## [0.17.0] - Support `aggregator_config` as `AggregatorConfig` variable name to simplify config system migration diff --git a/src/openeo_aggregator/about.py b/src/openeo_aggregator/about.py index 5b47833c..36a73a7b 100644 --- a/src/openeo_aggregator/about.py +++ b/src/openeo_aggregator/about.py @@ -2,7 +2,7 @@ import sys from typing import Optional -__version__ = "0.17.0a1" +__version__ = "0.18.0a1" def log_version_info(logger: Optional[logging.Logger] = None):