Fix problems with the node cache (#441)

* cache seems to work as intended now

* reenable tests

* fixing some issues

* reanble broken test

* remove unused code

.trunk/configs/.cspell.json

@@ -122,6 +122,7 @@
     "Doctest",
     "Doctests",
     "linenums",
-    "XLYOFNOQVPJJNP"
+    "XLYOFNOQVPJJNP",
+    "CRIPTUUID"
   ]
 }

src/cript/nodes/core.py

@@ -179,20 +179,13 @@ def _from_json(cls, json_dict: dict):
             if field_name not in arguments:
                 arguments[field_name] = getattr(default_dataclass, field_name)
 
-        # If a node with this UUID already exists, we don't create a new node.
-        # Instead we use the existing node from the cache and just update it.
-        from cript.nodes.uuid_base import UUIDBaseNode
-
-        if "uuid" in json_dict and json_dict["uuid"] in UUIDBaseNode._uuid_cache:
-            node = UUIDBaseNode._uuid_cache[json_dict["uuid"]]
-        else:  # Create a new node
-            try:
-                node = cls(**arguments)
-            # TODO we should not catch all exceptions if we are handling them, and instead let it fail
-            #  to create a good error message that points to the correct place that it failed to make debugging easier
-            except Exception as exc:
-                print(cls, arguments)
-                raise exc
+        try:
+            node = cls(**arguments)
+        # TODO we should not catch all exceptions if we are handling them, and instead let it fail
+        #  to create a good error message that points to the correct place that it failed to make debugging easier
+        except Exception as exc:
+            print(cls, arguments)
+            raise exc
 
         attrs = cls.JsonAttributes(**arguments)
 
@@ -213,6 +206,8 @@ def _from_json(cls, json_dict: dict):
         return node
 
     def __deepcopy__(self, memo):
+        from cript.nodes.util.core import get_uuid_from_uid
+
         # Ideally I would call `asdict`, but that is not allowed inside a deepcopy chain.
         # Making a manual transform into a dictionary here.
         arguments = {}
@@ -224,6 +219,8 @@ def __deepcopy__(self, memo):
         # a new uid will prompt the creation of a new matching uuid.
         uid = get_new_uid()
         arguments["uid"] = uid
+        if "uuid" in arguments:
+            arguments["uuid"] = get_uuid_from_uid(uid)
 
         # Create node and init constructor attributes
         node = self.__class__(**arguments)

src/cript/nodes/exceptions.py

@@ -4,6 +4,19 @@
 from cript.exceptions import CRIPTException
 
 
+class CRIPTUUIDException(CRIPTException):
+    def __init__(self, uuid: str, old_type: str, new_type: str):
+        self.uuid = uuid
+        self.old_type = old_type
+        self.new_type = new_type
+
+    def __str__(self) -> str:
+        return_msg = f"UUID collision error. A new node with UUID {self.uuid} is created of type {self.new_type},"
+        return_msg += f" but a node with the same UUID exists already as type {self.old_type}."
+        return_msg += " Please report the error on https://github.com/C-Accel-CRIPT/Python-SDK/issues , thank you."
+        return return_msg
+
+
 class CRIPTNodeSchemaError(CRIPTException):
     """
     ## Definition
@@ -335,27 +348,6 @@ def __str__(self) -> str:
         return ret_string
 
 
-def get_orphaned_experiment_exception(orphaned_node):
-    """
-    Return the correct specific Exception based in the orphaned node type for nodes not correctly listed in experiment.
-    """
-    from cript.nodes.primary_nodes.computation import Computation
-    from cript.nodes.primary_nodes.computation_process import ComputationProcess
-    from cript.nodes.primary_nodes.data import Data
-    from cript.nodes.primary_nodes.process import Process
-
-    if isinstance(orphaned_node, Data):
-        return CRIPTOrphanedDataError(orphaned_node)
-    if isinstance(orphaned_node, Process):
-        return CRIPTOrphanedProcessError(orphaned_node)
-    if isinstance(orphaned_node, Computation):
-        return CRIPTOrphanedComputationError(orphaned_node)
-    if isinstance(orphaned_node, ComputationProcess):
-        return CRIPTOrphanedComputationalProcessError(orphaned_node)
-    # Base case raise the parent exception. TODO add bug warning.
-    return CRIPTOrphanedExperimentError(orphaned_node)
-
-
 class CRIPTOrphanedDataError(CRIPTOrphanedExperimentError):
     """
     ## Definition

src/cript/nodes/primary_nodes/project.py

@@ -105,10 +105,8 @@ def __init__(self, name: str, collection: Optional[List[Collection]] = None, mat
         self._update_json_attrs_if_valid(new_json_attrs)
 
     def validate(self, api=None, is_patch=False, force_validation: bool = False):
-        from cript.nodes.exceptions import (
-            CRIPTOrphanedMaterialError,
-            get_orphaned_experiment_exception,
-        )
+        from cript.nodes.exceptions import CRIPTOrphanedMaterialError
+        from cript.nodes.util.core import get_orphaned_experiment_exception
 
         # First validate like other nodes
         super().validate(api=api, is_patch=is_patch, force_validation=force_validation)

src/cript/nodes/util/__init__.py

@@ -1,48 +1,9 @@
-from cript.nodes.exceptions import (
-    CRIPTOrphanedComputationalProcessError,
-    CRIPTOrphanedComputationError,
-    CRIPTOrphanedDataError,
-    CRIPTOrphanedMaterialError,
-    CRIPTOrphanedProcessError,
-)
-from cript.nodes.primary_nodes.experiment import Experiment
-from cript.nodes.primary_nodes.project import Project
-
 # trunk-ignore-begin(ruff/F401)
+from .core import (
+    add_orphaned_nodes_to_project,
+    get_orphaned_experiment_exception,
+    get_uuid_from_uid,
+)
 from .json import NodeEncoder, load_nodes_from_json
 
 # trunk-ignore-end(ruff/F401)
-
-
-def add_orphaned_nodes_to_project(project: Project, active_experiment: Experiment, max_iteration: int = -1):
-    """
-    Helper function that adds all orphaned material nodes of the project graph to the
-    `project.materials` attribute.
-    Material additions only is permissible with `active_experiment is None`.
-    This function also adds all orphaned data, process, computation and computational process nodes
-    of the project graph to the `active_experiment`.
-    This functions call `project.validate` and might raise Exceptions from there.
-    """
-    if active_experiment is not None and active_experiment not in project.find_children({"node": ["Experiment"]}):
-        raise RuntimeError(f"The provided active experiment {active_experiment} is not part of the project graph. Choose an active experiment that is part of a collection of this project.")
-
-    counter = 0
-    while True:
-        if counter > max_iteration >= 0:
-            break  # Emergency stop
-        try:
-            project.validate()
-        except CRIPTOrphanedMaterialError as exc:
-            # because calling the setter calls `validate` we have to force add the material.
-            project._json_attrs.material.append(exc.orphaned_node)
-        except CRIPTOrphanedDataError as exc:
-            active_experiment.data += [exc.orphaned_node]
-        except CRIPTOrphanedProcessError as exc:
-            active_experiment.process += [exc.orphaned_node]
-        except CRIPTOrphanedComputationError as exc:
-            active_experiment.computation += [exc.orphaned_node]
-        except CRIPTOrphanedComputationalProcessError as exc:
-            active_experiment.computation_process += [exc.orphaned_node]
-        else:
-            break
-        counter += 1

src/cript/nodes/util/core.py

@@ -0,0 +1,69 @@
+import uuid
+
+from cript.nodes.exceptions import (
+    CRIPTOrphanedComputationalProcessError,
+    CRIPTOrphanedComputationError,
+    CRIPTOrphanedDataError,
+    CRIPTOrphanedExperimentError,
+    CRIPTOrphanedMaterialError,
+    CRIPTOrphanedProcessError,
+)
+
+
+def get_uuid_from_uid(uid):
+    return str(uuid.UUID(uid[2:]))
+
+
+def add_orphaned_nodes_to_project(project, active_experiment, max_iteration: int = -1):
+    """
+    Helper function that adds all orphaned material nodes of the project graph to the
+    `project.materials` attribute.
+    Material additions only is permissible with `active_experiment is None`.
+    This function also adds all orphaned data, process, computation and computational process nodes
+    of the project graph to the `active_experiment`.
+    This functions call `project.validate` and might raise Exceptions from there.
+    """
+    if active_experiment is not None and active_experiment not in project.find_children({"node": ["Experiment"]}):
+        raise RuntimeError(f"The provided active experiment {active_experiment} is not part of the project graph. Choose an active experiment that is part of a collection of this project.")
+
+    counter = 0
+    while True:
+        if counter > max_iteration >= 0:
+            break  # Emergency stop
+        try:
+            project.validate()
+        except CRIPTOrphanedMaterialError as exc:
+            # because calling the setter calls `validate` we have to force add the material.
+            project._json_attrs.material.append(exc.orphaned_node)
+        except CRIPTOrphanedDataError as exc:
+            active_experiment.data += [exc.orphaned_node]
+        except CRIPTOrphanedProcessError as exc:
+            active_experiment.process += [exc.orphaned_node]
+        except CRIPTOrphanedComputationError as exc:
+            active_experiment.computation += [exc.orphaned_node]
+        except CRIPTOrphanedComputationalProcessError as exc:
+            active_experiment.computation_process += [exc.orphaned_node]
+        else:
+            break
+        counter += 1
+
+
+def get_orphaned_experiment_exception(orphaned_node):
+    """
+    Return the correct specific Exception based in the orphaned node type for nodes not correctly listed in experiment.
+    """
+    from cript.nodes.primary_nodes.computation import Computation
+    from cript.nodes.primary_nodes.computation_process import ComputationProcess
+    from cript.nodes.primary_nodes.data import Data
+    from cript.nodes.primary_nodes.process import Process
+
+    if isinstance(orphaned_node, Data):
+        return CRIPTOrphanedDataError(orphaned_node)
+    if isinstance(orphaned_node, Process):
+        return CRIPTOrphanedProcessError(orphaned_node)
+    if isinstance(orphaned_node, Computation):
+        return CRIPTOrphanedComputationError(orphaned_node)
+    if isinstance(orphaned_node, ComputationProcess):
+        return CRIPTOrphanedComputationalProcessError(orphaned_node)
+    # Base case raise the parent exception. TODO add bug warning.
+    return CRIPTOrphanedExperimentError(orphaned_node)

src/cript/nodes/util/json.py

@@ -14,6 +14,7 @@
     CRIPTJsonDeserializationError,
     CRIPTJsonNodeError,
 )
+from cript.nodes.uuid_base import UUIDBaseNode
 
 
 class NodeEncoder(json.JSONEncoder):
@@ -294,7 +295,7 @@ def __call__(self, node_str: Union[Dict, str]) -> Dict:
         return node_dict
 
 
-def load_nodes_from_json(nodes_json: Union[str, Dict]):
+def load_nodes_from_json(nodes_json: Union[str, Dict], _use_uuid_cache: Optional[Dict] = None):
     """
     User facing function, that return a node and all its children from a json string input.
 
@@ -349,7 +350,22 @@ def load_nodes_from_json(nodes_json: Union[str, Dict]):
     # but catches a lot of odd cases. And at the moment performance is not bottle necked here
     if not isinstance(nodes_json, str):
         nodes_json = json.dumps(nodes_json)
-    return json.loads(nodes_json, object_hook=node_json_hook)
+
+    # Store previous UUIDBaseNode Cache state
+    previous_uuid_cache = UUIDBaseNode._uuid_cache
+
+    if _use_uuid_cache is not None:  # If requested use a custom cache.
+        UUIDBaseNode._uuid_cache = _use_uuid_cache
+
+    try:
+        loaded_nodes = json.loads(nodes_json, object_hook=node_json_hook)
+    finally:
+        # Definitively restore the old cache state
+        UUIDBaseNode._uuid_cache = previous_uuid_cache
+
+    if _use_uuid_cache is not None:
+        return loaded_nodes, _use_uuid_cache
+    return loaded_nodes
 
 
 def _rename_field(serialize_dict: Dict, old_name: str, new_name: str) -> Dict:

src/cript/nodes/uuid_base.py

@@ -1,13 +1,10 @@
 import uuid
 from abc import ABC
 from dataclasses import dataclass, field, replace
-from typing import Any, Dict
+from typing import Any, Dict, Optional
 
 from cript.nodes.core import BaseNode
-
-
-def get_uuid_from_uid(uid):
-    return str(uuid.UUID(uid[2:]))
+from cript.nodes.exceptions import CRIPTUUIDException
 
 
 class UUIDBaseNode(BaseNode, ABC):
@@ -32,16 +29,26 @@ class JsonAttributes(BaseNode.JsonAttributes):
 
     _json_attrs: JsonAttributes = JsonAttributes()
 
+    def __new__(cls, *args, **kwargs):
+        uuid: Optional[str] = kwargs.get("uuid")
+        if uuid and uuid in UUIDBaseNode._uuid_cache:
+            existing_node_to_overwrite = UUIDBaseNode._uuid_cache[uuid]
+            if type(existing_node_to_overwrite) is not cls:
+                raise CRIPTUUIDException(uuid, type(existing_node_to_overwrite), cls)
+            return existing_node_to_overwrite
+        new_uuid_node = super().__new__(cls)
+        return new_uuid_node
+
     def __init__(self, **kwargs):
+        from cript.nodes.util.core import get_uuid_from_uid
+
         # initialize Base class with node
         super().__init__(**kwargs)
         # Respect uuid if passed as argument, otherwise construct uuid from uid
-        uuid = kwargs.get("uuid", get_uuid_from_uid(self.uid))
+        uuid: str = kwargs.get("uuid", get_uuid_from_uid(self.uid))
         # replace name and notes within PrimaryBase
         self._json_attrs = replace(self._json_attrs, uuid=uuid)
-
-        # Place successfully created node in the UUID cache
-        self._uuid_cache[uuid] = self
+        UUIDBaseNode._uuid_cache[uuid] = self
 
     @property
     def uuid(self) -> uuid.UUID:
@@ -54,11 +61,6 @@ def url(self):
         api = _get_global_cached_api()
         return f"{api.host}/{api.api_prefix}/{api.api_version}/{self.uuid}"
 
-    def __deepcopy__(self, memo):
-        node = super().__deepcopy__(memo)
-        node._json_attrs = replace(node._json_attrs, uuid=get_uuid_from_uid(node.uid))
-        return node
-
     @property
     def updated_by(self):
         return self._json_attrs.updated_by

tests/test_node_util.py

@@ -344,3 +344,23 @@ def test_expanded_json(complex_project_node):
     # raise CRIPTJsonDeserializationError
     with pytest.raises(cript.nodes.exceptions.CRIPTJsonDeserializationError):
         cript.load_nodes_from_json(condensed_json)
+
+
+def test_uuid_cache_override(complex_project_node):
+    normal_serial = complex_project_node.get_expanded_json()
+    reloaded_project = cript.load_nodes_from_json(normal_serial)
+
+    # For a normal load, the reloaded node as to be the same as before.
+    assert reloaded_project is complex_project_node
+
+    # Load with custom cache override
+    custom_project, cache = cript.load_nodes_from_json(normal_serial, _use_uuid_cache=dict())
+
+    assert custom_project is not reloaded_project
+
+    # Make sure that the nodes in the different caches are different
+    for key in cache:
+        old_node = cript.nodes.uuid_base.UUIDBaseNode._uuid_cache[key]
+        new_node = cache[key]
+        assert old_node.uuid == new_node.uuid
+        assert old_node is not new_node