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

openeo/util.py

soxofaan

some quick notes

openeo/rest/connection.py

openeo/rest/datacube.py

…ore compact, and remove get_schema_from_process_parameter

openeo/internal/processes/parse.py

openeo/rest/connection.py

openeo/rest/datacube.py

openeo/internal/processes/parse.py

openeo/rest/datacube.py

openeo/internal/processes/parse.py

openeo/util.py

tests/rest/datacube/test_datacube100.py

…Schema and add testing

…from-schema

soxofaan

a couple of additional notes

soxofaan · 2025-02-04T11:03:18Z

openeo/internal/processes/parse.py

+        elif isinstance(self.schema,dict):
+            if name in self.schema:
+                result = self.schema[name]
+        return result


This doesn't look compatible with how json schema works.

For example, this methods should allow to extract the options from these example use cases:

https://github.com/Open-EO/openeo-processes/blob/6141771129d58d2a292db3b91d80c812c49f4e52/resample_spatial.json#L79-L97

https://github.com/Open-EO/openeo-processes/blob/6141771129d58d2a292db3b91d80c812c49f4e52/proposals/sar_backscatter.json#L36-L51

https://github.com/Open-EO/openeo-processes/blob/6141771129d58d2a292db3b91d80c812c49f4e52/apply_kernel.json#L54-L67

For example, the caller should not have to specify a name. The implementation should extract the options from the "enum" listing(s) in the schema data

soxofaan · 2025-02-04T11:05:15Z

tests/internal/processes/test_parse.py

+        ],
+    )
+@pytest.mark.parametrize(("key", "expected"), [("x1", "y1"), ("x2", "y2"), ("x4", None)])
+def test_get_enum_options(schema, key, expected):


To parameterize this test, I would use more realistic use cases, e.g. the ones linked above

soxofaan · 2025-02-04T12:47:15Z

openeo/rest/datacube.py

-        ]
+        try:
+            schema = Process.from_dict(self.connection.describe_process("sar_backscatter")).get_parameter("coefficient").schema
+            coefficient_options = schema.get_enum_options("enum") + [None]


On top of getting the options, we should also get the default value from the schema. It is now hardcoded to "gamma0-terrain" (above), but that should also be dynamic

soxofaan · 2025-02-04T12:47:35Z

tests/rest/datacube/test_datacube100.py

+            "summary": "Computes backscatter from SAR input",
+            "parameters": [
+                {
+                    "default": "gamma0-terrain",


This test should also check that this default value is picked

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

openeo/util.py Outdated Show resolved Hide resolved

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

ElienVandermaesenVITO added 4 commits February 4, 2025 08:32

issue693 move adapt search_list_for_dict_key to get_enum_options for …

a4b79a0

…Schema and add testing

issue693 run pre-commit

454cf79

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

877c1a3

…from-schema

issue693 add test for sar_backscatter with coefficient check

d8a4718

ElienVandermaesenVITO requested a review from soxofaan February 4, 2025 08:11

soxofaan reviewed Feb 4, 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

soxofaan left a comment

soxofaan left a comment

soxofaan Feb 4, 2025

soxofaan Feb 4, 2025

soxofaan Feb 4, 2025

soxofaan Feb 4, 2025

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

soxofaan left a comment

Choose a reason for hiding this comment

soxofaan left a comment

Choose a reason for hiding this comment

soxofaan Feb 4, 2025

Choose a reason for hiding this comment

soxofaan Feb 4, 2025

Choose a reason for hiding this comment

soxofaan Feb 4, 2025

Choose a reason for hiding this comment

soxofaan Feb 4, 2025

Choose a reason for hiding this comment