dereference_from_node_arguments: add original node id to nodes

for Open-EO/openeo-python-driver#479

Author: soxofaan

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- `ProcessGraphVisitor.dereference_from_node_arguments()`: add original node id (by default) to the dereferenced nodes in the process graph ([Open-EO/openeo-python-driver#479](https://github.com/Open-EO/openeo-python-driver/issues/479))
+
 ### Removed
 
 ### Fixed

openeo/_version.py

@@ -1 +1 @@
-__version__ = "0.50.0a1"
+__version__ = "0.50.0a2"

openeo/internal/process_graph_visitor.py

@@ -12,10 +12,11 @@ class ProcessGraphVisitException(OpenEoClientException):
     pass
 
 
-# TODO: this key is non-standard and mainly specific to a particular openeo-python-driver use case,
+# TODO: these keys are non-standard and mainly specific to a particular openeo-python-driver use case,
 #       which probably should be refactored out from this more generic implementation.
 #       Making it a named constant at least makes this more visible and trackable.
 DEREFERENCED_NODE_KEY = "node"
+ORIG_NODE_ID_KEY = "_node_id"
 
 
 class ProcessGraphVisitor(ABC):
@@ -27,18 +28,23 @@ def __init__(self):
         self.process_stack = []
 
     @classmethod
-    def dereference_from_node_arguments(cls, process_graph: dict) -> str:
+    def dereference_from_node_arguments(cls, process_graph: dict, add_node_id: bool = True) -> str:
         """
         Walk through the given (flat) process graph and replace (in-place) "from_node" references in
         process arguments (dictionaries or lists) with the corresponding resolved subgraphs
 
         :param process_graph: process graph dictionary (flat representation) to be manipulated in-place
+        :param add_node_id: whether to add the original node id in the node dict (alongside "process_id", "arguments", ...)
         :return: name of the "result" node of the graph
         """
 
         # TODO avoid manipulating process graph in place? make it more explicit? work on a copy?
         # TODO call it more something like "unflatten"?. Split this off of ProcessGraphVisitor?
         # TODO implement this through `ProcessGraphUnflattener` ?
+        # TODO the "there can only be one result node" view is outdated and is getting counter-produtive
+        #      or more generally, the dereferencing aspect in the current process graph processing approach
+        #      makes some things unnecessarily complicated (e.g. caching, or trying to preserve
+        #      the original node id per https://github.com/Open-EO/openeo-python-driver/issues/479)
 
         def resolve_from_node(process_graph: dict, node_id: str, from_node: str):
             if from_node not in process_graph:
@@ -53,6 +59,8 @@ def resolve_from_node(process_graph: dict, node_id: str, from_node: str):
                 if result_node:
                     raise ProcessGraphVisitException(f"Multiple result nodes: {result_node!r}, {node_id!r}")
                 result_node = node_id
+            if add_node_id:
+                node_data[ORIG_NODE_ID_KEY] = node_id
             arguments = node_data.get("arguments", {})
             for arg in arguments.values():
                 if isinstance(arg, dict):

tests/internal/test_process_graph_visitor.py

@@ -148,12 +148,34 @@ def test_visit_array_with_dereferenced_nodes():
                 "process_id": "array_element",
                 "arguments": {"data": {"from_parameter": "data"}, "index": 2},
                 "result": False,
+                "_node_id": "arrayelement1",
             }
         )
     ]
     assert visitor.constantArrayElement.call_args_list == [call(-1)]
 
 
+def test_dereference_simple():
+    graph = {
+        "foo1": {
+            "process_id": "foo",
+            "arguments": {"bar": 123},
+            "result": True,
+        }
+    }
+    result = ProcessGraphVisitor.dereference_from_node_arguments(graph)
+
+    assert result == "foo1"
+    assert graph == {
+        "foo1": {
+            "process_id": "foo",
+            "arguments": {"bar": 123},
+            "result": True,
+            "_node_id": "foo1",
+        },
+    }
+
+
 def test_dereference_basic():
     graph = {
         "node1": {},
@@ -184,25 +206,39 @@ def test_dereference_basic():
     assert graph["node3"] == graph["node2"]["arguments"]["data2"]["node"]
     assert graph["node4"] == graph["node3"]["arguments"]["data"]["node"]
     assert graph == {
-        "node1": {},
+        "node1": {"_node_id": "node1"},
         "node2": {
             "arguments": {
-                "data1": {"from_node": "node1", "node": {}},
-                "data2": {"from_node": "node3", "node": {
-                    "arguments": {
-                        "data": {"from_node": "node4", "node": {}},
-                    }
-                }},
+                "data1": {
+                    "from_node": "node1",
+                    "node": {"_node_id": "node1"},
+                },
+                "data2": {
+                    "from_node": "node3",
+                    "node": {
+                        "arguments": {
+                            "data": {
+                                "from_node": "node4",
+                                "node": {"_node_id": "node4"},
+                            },
+                        },
+                        "_node_id": "node3",
+                    },
+                },
             },
-            "result": True
+            "result": True,
+            "_node_id": "node2",
         },
         "node3": {
             "arguments": {
-                "data": {"from_node": "node4", "node": {}},
-            }
+                "data": {
+                    "from_node": "node4",
+                    "node": {"_node_id": "node4"},
+                },
+            },
+            "_node_id": "node3",
         },
-        "node4": {}
-
+        "node4": {"_node_id": "node4"},
     }
 
 
@@ -221,17 +257,34 @@ def test_dereference_list_arg():
     result = ProcessGraphVisitor.dereference_from_node_arguments(graph)
     assert result == "temporal"
     assert graph == {
-        "start": {"process_id": "constant", "arguments": {"x": "2020-02-02"}},
-        "end": {"process_id": "constant", "arguments": {"x": "2020-03-03"}},
+        "start": {
+            "process_id": "constant",
+            "arguments": {"x": "2020-02-02"},
+            "_node_id": "start",
+        },
+        "end": {
+            "process_id": "constant",
+            "arguments": {"x": "2020-03-03"},
+            "_node_id": "end",
+        },
         "temporal": {
             "process_id": "filter_temporal",
             "arguments": {
                 "extent": [
-                    {"process_id": "constant", "arguments": {"x": "2020-02-02"}},
-                    {"process_id": "constant", "arguments": {"x": "2020-03-03"}},
+                    {
+                        "process_id": "constant",
+                        "arguments": {"x": "2020-02-02"},
+                        "_node_id": "start",
+                    },
+                    {
+                        "process_id": "constant",
+                        "arguments": {"x": "2020-03-03"},
+                        "_node_id": "end",
+                    },
                 ],
             },
             "result": True,
+            "_node_id": "temporal",
         },
     }
 
@@ -254,24 +307,41 @@ def test_dereference_dict_arg():
     result = ProcessGraphVisitor.dereference_from_node_arguments(graph)
     assert result == "bbox"
     assert graph == {
-        "west": {"process_id": "add", "arguments": {"x": 1, "y": 1}},
-        "east": {"process_id": "add", "arguments": {"x": 2, "y": 3}},
+        "west": {
+            "process_id": "add",
+            "arguments": {"x": 1, "y": 1},
+            "_node_id": "west",
+        },
+        "east": {
+            "process_id": "add",
+            "arguments": {"x": 2, "y": 3},
+            "_node_id": "east",
+        },
         "bbox": {
             "process_id": "filter_bbox",
             "arguments": {
                 "extent": {
                     "west": {
                         "from_node": "west",
-                        "node": {"process_id": "add", "arguments": {"x": 1, "y": 1}},
+                        "node": {
+                            "process_id": "add",
+                            "arguments": {"x": 1, "y": 1},
+                            "_node_id": "west",
+                        },
                     },
                     "east": {
                         "from_node": "east",
-                        "node": {"process_id": "add", "arguments": {"x": 2, "y": 3}},
+                        "node": {
+                            "process_id": "add",
+                            "arguments": {"x": 2, "y": 3},
+                            "_node_id": "east",
+                        },
                     },
                 }
             },
             "result": True,
-        }
+            "_node_id": "bbox",
+        },
     }
 
 
@@ -326,6 +396,77 @@ def test_dereference_cycle():
     assert graph["node2"]["arguments"]["data"]["node"] is graph["node1"]
 
 
+@pytest.mark.parametrize(
+    ["add_node_id", "expected"],
+    [
+        (
+            False,
+            {
+                "foo1": {
+                    "process_id": "foo",
+                    "arguments": {"bar": 123},
+                },
+                "bar1": {
+                    "process_id": "bar",
+                    "arguments": {
+                        "foo": {
+                            "from_node": "foo1",
+                            "node": {
+                                "process_id": "foo",
+                                "arguments": {"bar": 123},
+                            },
+                        }
+                    },
+                    "result": True,
+                },
+            },
+        ),
+        (
+            True,
+            {
+                "foo1": {
+                    "process_id": "foo",
+                    "arguments": {"bar": 123},
+                    "_node_id": "foo1",
+                },
+                "bar1": {
+                    "process_id": "bar",
+                    "arguments": {
+                        "foo": {
+                            "from_node": "foo1",
+                            "node": {
+                                "process_id": "foo",
+                                "arguments": {"bar": 123},
+                                "_node_id": "foo1",
+                            },
+                        }
+                    },
+                    "result": True,
+                    "_node_id": "bar1",
+                },
+            },
+        ),
+    ],
+)
+def test_dereference_add_node_id(add_node_id, expected):
+    graph = {
+        "foo1": {
+            "process_id": "foo",
+            "arguments": {"bar": 123},
+        },
+        "bar1": {
+            "process_id": "bar",
+            "arguments": {"foo": {"from_node": "foo1"}},
+            "result": True,
+        },
+    }
+    result = ProcessGraphVisitor.dereference_from_node_arguments(graph, add_node_id=add_node_id)
+
+    assert result == "bar1"
+    assert graph == expected
+
+
+
 class TestProcessGraphUnflattener:
     def test_minimal(self):
         graph = {