Merge branch 'issue404_automatically-validate-process-graph-before-download-or-execute'

Author: soxofaan

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 - Add `DataCube.reduce_spatial()`
+- Added option (enabled by default) to automatically validate a process graph before execution. Validation issues just trigger warnings for now. ([#404](https://github.com/Open-EO/openeo-python-client/issues/404))
 
 ### Changed
 

openeo/rest/_testing.py

@@ -1,3 +1,4 @@
+import json
 import re
 from typing import Optional, Union
 
@@ -11,16 +12,33 @@ class DummyBackend:
     and allows inspection of posted process graphs
     """
 
+    __slots__ = (
+        "connection",
+        "sync_requests",
+        "batch_jobs",
+        "validation_requests",
+        "next_result",
+        "next_validation_errors",
+    )
+
     # Default result (can serve both as JSON or binary data)
     DEFAULT_RESULT = b'{"what?": "Result data"}'
 
     def __init__(self, requests_mock, connection: Connection):
         self.connection = connection
         self.sync_requests = []
         self.batch_jobs = {}
+        self.validation_requests = []
         self.next_result = self.DEFAULT_RESULT
-        requests_mock.post(connection.build_url("/result"), content=self._handle_post_result)
-        requests_mock.post(connection.build_url("/jobs"), content=self._handle_post_jobs)
+        self.next_validation_errors = []
+        requests_mock.post(
+            connection.build_url("/result"),
+            content=self._handle_post_result,
+        )
+        requests_mock.post(
+            connection.build_url("/jobs"),
+            content=self._handle_post_jobs,
+        )
         requests_mock.post(
             re.compile(connection.build_url(r"/jobs/(job-\d+)/results$")), content=self._handle_post_job_results
         )
@@ -32,12 +50,19 @@ def __init__(self, requests_mock, connection: Connection):
             re.compile(connection.build_url("/jobs/(.*?)/results/result.data$")),
             content=self._handle_get_job_result_asset,
         )
+        requests_mock.post(connection.build_url("/validation"), json=self._handle_post_validation)
 
     def _handle_post_result(self, request, context):
         """handler of `POST /result` (synchronous execute)"""
         pg = request.json()["process"]["process_graph"]
         self.sync_requests.append(pg)
-        return self.next_result
+        result = self.next_result
+        if isinstance(result, (dict, list)):
+            result = json.dumps(result).encode("utf-8")
+        elif isinstance(result, str):
+            result = result.encode("utf-8")
+        assert isinstance(result, bytes)
+        return result
 
     def _handle_post_jobs(self, request, context):
         """handler of `POST /jobs` (create batch job)"""
@@ -83,6 +108,12 @@ def _handle_get_job_result_asset(self, request, context):
         assert self.batch_jobs[job_id]["status"] == "finished"
         return self.next_result
 
+    def _handle_post_validation(self, request, context):
+        """Handler of `POST /validation` (validate process graph)."""
+        pg = request.json()["process_graph"]
+        self.validation_requests.append(pg)
+        return {"errors": self.next_validation_errors}
+
     def get_sync_pg(self) -> dict:
         """Get one and only synchronous process graph"""
         assert len(self.sync_requests) == 1

openeo/rest/connection.py

@@ -260,6 +260,7 @@ def __init__(
         refresh_token_store: Optional[RefreshTokenStore] = None,
         slow_response_threshold: Optional[float] = None,
         oidc_auth_renewer: Optional[OidcAuthenticator] = None,
+        auto_validate: bool = True,
     ):
         """
         Constructor of Connection, authenticates user.
@@ -282,6 +283,7 @@ def __init__(
         self._auth_config = auth_config
         self._refresh_token_store = refresh_token_store
         self._oidc_auth_renewer = oidc_auth_renewer
+        self._auto_validate = auto_validate
 
     @classmethod
     def version_discovery(
@@ -1052,8 +1054,8 @@ def validate_process_graph(self, process_graph: dict) -> List[dict]:
         :param process_graph: (flat) dict representing process graph
         :return: list of errors (dictionaries with "code" and "message" fields)
         """
-        request = {"process_graph": process_graph}
-        return self.post(path="/validation", json=request, expected_status=200).json()["errors"]
+        pg_with_metadata = self._build_request_with_process_graph(process_graph)["process"]
+        return self.post(path="/validation", json=pg_with_metadata, expected_status=200).json()["errors"]
 
     @property
     def _api_version(self) -> ComparableVersion:
@@ -1393,8 +1395,9 @@ def load_url(self, url: str, format: str, options: Optional[dict] = None):
 
     def create_service(self, graph: dict, type: str, **kwargs) -> Service:
         # TODO: type hint for graph: is it a nested or a flat one?
-        req = self._build_request_with_process_graph(process_graph=graph, type=type, **kwargs)
-        response = self.post(path="/services", json=req, expected_status=201)
+        pg_with_metadata = self._build_request_with_process_graph(process_graph=graph, type=type, **kwargs)
+        self._preflight_validation(pg_with_metadata=pg_with_metadata)
+        response = self.post(path="/services", json=pg_with_metadata, expected_status=201)
         service_id = response.headers.get("OpenEO-Identifier")
         return Service(service_id, self)
 
@@ -1463,23 +1466,55 @@ def upload_file(
     def _build_request_with_process_graph(self, process_graph: Union[dict, FlatGraphableMixin, Any], **kwargs) -> dict:
         """
         Prepare a json payload with a process graph to submit to /result, /services, /jobs, ...
-        :param process_graph: flat dict representing a process graph
+        :param process_graph: flat dict representing a "process graph with metadata" ({"process": {"process_graph": ...}, ...})
         """
         # TODO: make this a more general helper (like `as_flat_graph`)
         result = kwargs
         process_graph = as_flat_graph(process_graph)
         if "process_graph" not in process_graph:
             process_graph = {"process_graph": process_graph}
-        # TODO: also check if `process_graph` already has "process" key (i.e. is a "process graph with metadata already)
+        # TODO: also check if `process_graph` already has "process" key (i.e. is a "process graph with metadata" already)
         result["process"] = process_graph
         return result
 
+    def _preflight_validation(self, pg_with_metadata: dict, *, validate: Optional[bool] = None):
+        """
+        Preflight validation of process graph to execute.
+
+        :param pg_with_metadata: flat dict representation of process graph with metadata,
+            e.g. as produced by `_build_request_with_process_graph`
+        :param validate: Optional toggle to enable/prevent validation of the process graphs before execution
+            (overruling the connection's ``auto_validate`` setting).
+
+        :return:
+        """
+        if validate is None:
+            validate = self._auto_validate
+        if validate and self.capabilities().supports_endpoint("/validation", "POST"):
+            # At present, the intention is that a failed validation does not block
+            # the job from running, it is only reported as a warning.
+            # Therefor we also want to continue when something *else* goes wrong
+            # *during* the validation.
+            try:
+                resp = self.post(path="/validation", json=pg_with_metadata["process"], expected_status=200)
+                validation_errors = resp.json()["errors"]
+                if validation_errors:
+                    _log.warning(
+                        "Preflight process graph validation raised: "
+                        + (" ".join(f"[{e.get('code')}] {e.get('message')}" for e in validation_errors))
+                    )
+            except Exception as e:
+                _log.error(f"Preflight process graph validation failed: {e}", exc_info=True)
+
+        # TODO: additional validation and sanity checks: e.g. is there a result node, are all process_ids valid, ...?
+
     # TODO: unify `download` and `execute` better: e.g. `download` always writes to disk, `execute` returns result (raw or as JSON decoded dict)
     def download(
         self,
         graph: Union[dict, FlatGraphableMixin, str, Path],
         outputfile: Union[Path, str, None] = None,
         timeout: Optional[int] = None,
+        validate: Optional[bool] = None,
     ) -> Union[None, bytes]:
         """
         Downloads the result of a process graph synchronously,
@@ -1490,11 +1525,14 @@ def download(
             or as local file path or URL
         :param outputfile: output file
         :param timeout: timeout to wait for response
+        :param validate: Optional toggle to enable/prevent validation of the process graphs before execution
+            (overruling the connection's ``auto_validate`` setting).
         """
-        request = self._build_request_with_process_graph(process_graph=graph)
+        pg_with_metadata = self._build_request_with_process_graph(process_graph=graph)
+        self._preflight_validation(pg_with_metadata=pg_with_metadata, validate=validate)
         response = self.post(
             path="/result",
-            json=request,
+            json=pg_with_metadata,
             expected_status=200,
             stream=True,
             timeout=timeout or DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE,
@@ -1511,21 +1549,26 @@ def execute(
         self,
         process_graph: Union[dict, str, Path],
         timeout: Optional[int] = None,
+        validate: Optional[bool] = None,
     ):
         """
         Execute a process graph synchronously and return the result (assumed to be JSON).
 
         :param process_graph: (flat) dict representing a process graph, or process graph as raw JSON string,
             or as local file path or URL
+        :param validate: Optional toggle to enable/prevent validation of the process graphs before execution
+            (overruling the connection's ``auto_validate`` setting).
+
         :return: parsed JSON response
         """
-        req = self._build_request_with_process_graph(process_graph=process_graph)
+        pg_with_metadata = self._build_request_with_process_graph(process_graph=process_graph)
+        self._preflight_validation(pg_with_metadata=pg_with_metadata, validate=validate)
         return self.post(
             path="/result",
-            json=req,
+            json=pg_with_metadata,
             expected_status=200,
             timeout=timeout or DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE,
-        ).json()
+        ).json()  # TODO: only do JSON decoding when mimetype is actually JSON?
 
     def create_job(
         self,
@@ -1536,6 +1579,7 @@ def create_job(
         plan: Optional[str] = None,
         budget: Optional[float] = None,
         additional: Optional[dict] = None,
+        validate: Optional[bool] = None,
     ) -> BatchJob:
         """
         Create a new job from given process graph on the back-end.
@@ -1547,18 +1591,22 @@ def create_job(
         :param plan: billing plan
         :param budget: maximum cost the request is allowed to produce
         :param additional: additional job options to pass to the backend
+        :param validate: Optional toggle to enable/prevent validation of the process graphs before execution
+            (overruling the connection's ``auto_validate`` setting).
         :return: Created job
         """
         # TODO move all this (BatchJob factory) logic to BatchJob?
-        req = self._build_request_with_process_graph(
+
+        pg_with_metadata = self._build_request_with_process_graph(
             process_graph=process_graph,
             **dict_no_none(title=title, description=description, plan=plan, budget=budget)
         )
         if additional:
             # TODO: get rid of this non-standard field? https://github.com/Open-EO/openeo-api/issues/276
-            req["job_options"] = additional
+            pg_with_metadata["job_options"] = additional
 
-        response = self.post("/jobs", json=req, expected_status=201)
+        self._preflight_validation(pg_with_metadata=pg_with_metadata, validate=validate)
+        response = self.post("/jobs", json=pg_with_metadata, expected_status=201)
 
         job_id = None
         if "openeo-identifier" in response.headers:
@@ -1636,8 +1684,8 @@ def as_curl(
         cmd += ["-H", "Content-Type: application/json"]
         if isinstance(self.auth, BearerAuth):
             cmd += ["-H", f"Authorization: Bearer {'...' if obfuscate_auth else self.auth.bearer}"]
-        post_data = self._build_request_with_process_graph(data)
-        post_json = json.dumps(post_data, separators=(',', ':'))
+        pg_with_metadata = self._build_request_with_process_graph(data)
+        post_json = json.dumps(pg_with_metadata, separators=(",", ":"))
         cmd += ["--data", post_json]
         cmd += [self.build_url(path)]
         return " ".join(shlex.quote(c) for c in cmd)
@@ -1657,17 +1705,20 @@ def version_info(self):
 
 
 def connect(
-        url: Optional[str] = None,
-        auth_type: Optional[str] = None, auth_options: Optional[dict] = None,
-        session: Optional[requests.Session] = None,
-        default_timeout: Optional[int] = None,
+    url: Optional[str] = None,
+    *,
+    auth_type: Optional[str] = None,
+    auth_options: Optional[dict] = None,
+    session: Optional[requests.Session] = None,
+    default_timeout: Optional[int] = None,
+    auto_validate: bool = True,
 ) -> Connection:
     """
     This method is the entry point to OpenEO.
     You typically create one connection object in your script or application
     and re-use it for all calls to that backend.
 
-    If the backend requires authentication, you can pass authentication data directly to this function
+    If the backend requires authentication, you can pass authentication data directly to this function,
     but it could be easier to authenticate as follows:
 
         >>> # For basic authentication
@@ -1679,7 +1730,10 @@ def connect(
     :param auth_type: Which authentication to use: None, "basic" or "oidc" (for OpenID Connect)
     :param auth_options: Options/arguments specific to the authentication type
     :param default_timeout: default timeout (in seconds) for requests
-    :rtype: openeo.connections.Connection
+    :param auto_validate: toggle to automatically validate process graphs before execution
+
+    .. versionadded:: 0.24.0
+        added ``auto_validate`` argument
     """
 
     def _config_log(message):
@@ -1704,7 +1758,7 @@ def _config_log(message):
 
     if not url:
         raise OpenEoClientException("No openEO back-end URL given or known to connect to.")
-    connection = Connection(url, session=session, default_timeout=default_timeout)
+    connection = Connection(url, session=session, default_timeout=default_timeout, auto_validate=auto_validate)
 
     auth_type = auth_type.lower() if isinstance(auth_type, str) else auth_type
     if auth_type in {None, False, 'null', 'none'}: