issue #693 add search for schema and find parameters from the backend #698

ElienVandermaesenVITO · 2025-01-06T06:55:56Z

soxofaan

In this PR you add a lot of utility functions. A couple of notes on that:

they are al public at the moment, so adding them adds the burden to keep/maintain/support them. In that sense, they should also get good test coverage.
some of the seem quite tailored for the specific use case of this PR, while they are named very generically

also note that we already have parsing functionality for process definitions: e.g. see starting point openeo.internal.processes.Process.from_dict(). I think that should eliminate the need for all these new utility functions

I think it's also important that the core goal of this PR gets unit tests coverage

openeo/rest/connection.py

openeo/rest/datacube.py

ElienVandermaesenVITO · 2025-01-09T09:29:34Z

The test fail because of No mock address: GET https://oeo.test/processes. Do I have to change the test and add the address or is there another way to solve it?

soxofaan · 2025-01-09T10:30:22Z

this feature indeed impacts existing tests

FAILED tests/rest/datacube/test_datacube100.py::test_sar_backscatter_defaults - requests_mock.exceptions.NoMockAddress: No mock address: GET https://oeo.test/processes
FAILED tests/rest/datacube/test_datacube100.py::test_sar_backscatter_custom - requests_mock.exceptions.NoMockAddress: No mock address: GET https://oeo.test/processes
FAILED tests/rest/datacube/test_datacube100.py::test_sar_backscatter_coefficient_none - requests_mock.exceptions.NoMockAddress: No mock address: GET https://oeo.test/processes
FAILED tests/rest/datacube/test_datacube100.py::test_sar_backscatter_coefficient_invalid - requests_mock.exceptions.NoMockAddress: No mock address: GET https://oeo.test/processes

doing cube.sar_backscatter() now triggers a GET /processes request, so you should indeed add a mock for that in these tests

…from-schema

ElienVandermaesenVITO · 2025-01-09T13:07:47Z

openeo/util.py

@@ -687,3 +687,22 @@ def normalize_crs(crs: Any, *, use_pyproj: bool = True) -> Union[None, int, str]
            raise ValueError(f"Can not normalize CRS data {type(crs)}")

    return crs
+
+
+def search_list_for_dict_key(lst: list, key: str) -> Union[dict, list]:


Moved this function to util.py, because it couldn't be imported from connection.py due to a circular import issue. Can it stay here or should it be moved elsewhere @soxofaan

instead of this generic utility, I would define a method get_enum_options() on the Schema object

soxofaan

some quick notes

openeo/rest/connection.py

openeo/rest/datacube.py

…ore compact, and remove get_schema_from_process_parameter

soxofaan · 2025-01-22T13:20:14Z

openeo/internal/processes/parse.py

@@ -127,6 +127,12 @@ def from_json_file(cls, path: Union[str, Path]) -> Process:
        with Path(path).open("r") as f:
            return cls.from_json(f.read())

+    def get_parameter(self, parameter_id: str) -> Union[dict, list]:


can you add a test for this new method?

Also to stay closer to the openEO spec, I would just use name as argument here. "Parameter Id" is not really a thing in the spec

soxofaan · 2025-01-22T13:25:27Z

openeo/rest/connection.py

@@ -38,6 +38,7 @@
 from openeo.internal.graph_building import FlatGraphableMixin, PGNode, as_flat_graph
 from openeo.internal.jupyter import VisualDict, VisualList
 from openeo.internal.processes.builder import ProcessBuilderBase
+from openeo.internal.processes.parse import Process


this adds an unused import I think

soxofaan · 2025-01-22T13:27:18Z

openeo/rest/datacube.py

+            schema = Process.from_dict(self.connection.describe_process("sar_backscatter")).get_parameter("coefficient")
+            coefficient_options = search_list_for_dict_key(schema, "enum") + [None]
+        except Exception:
+            log.warning(f"Failed to extract option for coefficient in sar_backscatter")


Suggested change

log.warning(f"Failed to extract option for coefficient in sar_backscatter")

log.warning(f"Failed to extract coefficient options for process `sar_backscatter`: {e}")

soxofaan · 2025-01-22T13:27:41Z

openeo/internal/processes/parse.py

+        for parameter in self.parameters:
+            if parameter.name == parameter_id:
+                return parameter.schema.schema
+        raise LookupError(f"Expected parameter {parameter_id} not found.")


Suggested change

raise LookupError(f"Expected parameter {parameter_id} not found.")

raise LookupError(f"Parameter {parameter_id!r} not found.")

soxofaan · 2025-01-22T13:28:06Z

openeo/rest/datacube.py

+        try:
+            schema = Process.from_dict(self.connection.describe_process("sar_backscatter")).get_parameter("coefficient")
+            coefficient_options = search_list_for_dict_key(schema, "enum") + [None]
+        except Exception:


Suggested change

except Exception:

except Exception as e:

soxofaan · 2025-01-22T13:30:35Z

openeo/internal/processes/parse.py

+    def get_parameter(self, parameter_id: str) -> Union[dict, list]:
+        for parameter in self.parameters:
+            if parameter.name == parameter_id:
+                return parameter.schema.schema


I would return the whole parameter instead of just its schema to be more generic

Suggested change

return parameter.schema.schema

return parameter

soxofaan · 2025-01-22T13:35:44Z

openeo/util.py

@@ -687,3 +687,22 @@ def normalize_crs(crs: Any, *, use_pyproj: bool = True) -> Union[None, int, str]
            raise ValueError(f"Can not normalize CRS data {type(crs)}")

    return crs
+
+
+def search_list_for_dict_key(lst: list, key: str) -> Union[dict, list]:


instead of this generic utility, I would define a method get_enum_options() on the Schema object

soxofaan · 2025-01-22T13:38:46Z

tests/rest/datacube/test_datacube100.py

+            "returns": {"description": "incremented value", "schema": {"type": "integer"}},
+        }
+    ]
+    requests_mock.get(API_URL + "/processes", json={"processes": processes})


I would not add these request_mock to the existing tests, because these tests have to keep working without change.

Instead, I would add new tests where a custom sar_backscatter is mocked.

issue #693 add search for schema and find parameters from the backend

488262f

ElienVandermaesenVITO requested a review from jdries January 6, 2025 06:59

soxofaan reviewed Jan 6, 2025

View reviewed changes

openeo/rest/connection.py Outdated Show resolved Hide resolved

openeo/rest/connection.py Outdated Show resolved Hide resolved

openeo/rest/connection.py Outdated Show resolved Hide resolved

openeo/rest/datacube.py Outdated Show resolved Hide resolved

ElienVandermaesenVITO added 3 commits January 8, 2025 15:46

issue #693 use openeo.internal.process.parse intead of new functions

8699f3c

issue #698 add exception and change documentation

7440d71

issue #698 add fallback situations

b506a11

ElienVandermaesenVITO added 2 commits January 9, 2025 13:57

issue #693 add mock for sar_backscatter test for processes

71b2b79

Merge branch 'master' into issue693-sar_backscatter-get-coefficients-…

53936a6

…from-schema

ElienVandermaesenVITO commented Jan 9, 2025

View reviewed changes

issue #693 add test for search_list_for_dict_key

ece05dc

soxofaan reviewed Jan 13, 2025

View reviewed changes

issue #693 add parse.get_parameter, to retrieve parameter in schema m…

503154b

…ore compact, and remove get_schema_from_process_parameter

ElienVandermaesenVITO requested a review from soxofaan January 14, 2025 13:42

soxofaan reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #693 add search for schema and find parameters from the backend #698

issue #693 add search for schema and find parameters from the backend #698

ElienVandermaesenVITO commented Jan 6, 2025

soxofaan left a comment

ElienVandermaesenVITO commented Jan 9, 2025

soxofaan commented Jan 9, 2025

ElienVandermaesenVITO Jan 9, 2025 •

edited

Loading

soxofaan Jan 22, 2025

soxofaan left a comment

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

soxofaan Jan 22, 2025

	log.warning(f"Failed to extract option for coefficient in sar_backscatter")
	log.warning(f"Failed to extract coefficient options for process `sar_backscatter`: {e}")

	raise LookupError(f"Expected parameter {parameter_id} not found.")
	raise LookupError(f"Parameter {parameter_id!r} not found.")

issue #693 add search for schema and find parameters from the backend #698

Are you sure you want to change the base?

issue #693 add search for schema and find parameters from the backend #698

Conversation

ElienVandermaesenVITO commented Jan 6, 2025

soxofaan left a comment

Choose a reason for hiding this comment

ElienVandermaesenVITO commented Jan 9, 2025

soxofaan commented Jan 9, 2025

ElienVandermaesenVITO Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soxofaan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElienVandermaesenVITO Jan 9, 2025 •

edited

Loading