Implement new link types #2220

sphuber · 2018-11-20T15:17:46Z

With the new ORM hierarchy in place, the corresponding link types can be defined:

CREATE: CalculationNode -> Data
RETURN: WorkflowNode -> Data
INPUT_CALC: Data -> CalculationNode
INPUT_WORK: Data -> WorkflowNode
CALL_CALC: WorkflowNode -> CalculationNode
CALL_WORK: WorkflowNode -> WorkflowNode

These rules are encoded in the validate_link function in aiida.orm.utils.links.
This function is called from add_link_from, which has been renamed to add_incoming,
and validates the triple (source, link, target) representing the addition of a link
from a source node to a target node.

coveralls · 2018-11-20T15:36:20Z

Coverage increased (+6.9%) to 68.98% when pulling 834e27a on sphuber:fix_2177_implement_new_link_types into f2ee588 on aiidateam:provenance_redesign.

With the new ORM hierarchy in place, the corresponding link types can be defined: CREATE: CalculationNode -> Data RETURN: WorkflowNode -> Data INPUT_CALC: Data -> CalculationNode INPUT_WORK: Data -> WorkflowNode CALL_CALC: WorkflowNode -> CalculationNode CALL_WORK: WorkflowNode -> WorkflowNode These rules are encoded in the `validate_link` function in `aiida.orm.utils.links`. This function is called from `add_link_from`, which will be renamed to `add_incoming`, and validate the triple (source, link, target) representing the addition of a link from a source node to a target node.

This is to align it with the recent implementation of `get_incoming` and `get_outgoing` to obtain all incoming and outgoing links, respectively.

muhrin · 2018-11-23T10:01:29Z

aiida/orm/utils/links.py

+    if not isinstance(source, type_source) or not isinstance(target, type_target):
+        raise ValueError('cannot add a {} link from {} to {}'.format(link_type, type(source), type(target)))
+
+    incoming = dict(target.get_inputs(node_type=type(source), link_type=link_type, also_labels=True))


Use get_incoming instead

muhrin · 2018-11-23T10:26:29Z

aiida/orm/utils/links.py

+        raise ValueError('node<{}> already has an incoming {} link'.format(target.uuid, link_type))
+
+    # If the indegree is `unique_label` than the pair (source, label) should be unique for the target node
+    if type_degree == 'unique_label' and (source.uuid, link_label) in [(n.uuid, l) for l, n in incoming.items()]:


Consider changing to just check the link labels for uniqueness at this point.

muhrin · 2018-11-23T10:29:40Z

aiida/backends/tests/cmdline/commands/test_work.py

@@ -36,7 +39,7 @@ def test_status(self):
        """Test the status command."""
        calc = WorkChainNode().store()
        result = self.cli_runner.invoke(cmd_work.work_status, [str(calc.pk)])
-        self.assertIsNone(result.exception, result.output)
+        self.assertIsNone(result.exception, ''.join(traceback.format_exception(*result.exc_info)))


Consider using self.assertClickResultNoException

muhrin · 2018-11-23T10:31:41Z

aiida/backends/tests/orm/node/test_node.py

+
+        # Even when the source node is different
+        with self.assertRaises(ValueError):
+            target.validate_incoming(source_two, LinkType.CREATE)


Add a case where link label is different

aiida/backends/tests/orm/node/test_node.py

muhrin · 2018-11-23T10:39:24Z

aiida/orm/implementation/sqlalchemy/node.py

@@ -280,7 +280,7 @@ def _add_dblink_from(self, src, label=None, link_type=LinkType.UNSPECIFIED):
        #
        # I am linking src->self; a loop would be created if a DbPath exists
        # already in the TC table from self to src
-        if link_type is LinkType.CREATE or link_type is LinkType.INPUT:
+        if link_type is LinkType.CREATE or link_type is LinkType.INPUT_CALC or link_type is LinkType.INPUT_WORK:


Leave checks out from backend classes

muhrin · 2018-11-23T10:42:48Z

aiida/orm/node/process/calculation/calculation.py

+        """
+        super(CalculationNode, self).validate_incoming(source, link_type, link_label)
+        from aiida.orm.utils.links import validate_link
+        validate_link(source, self, link_type, link_label)


Is this needed? Not already in Node?

I remember now, this needs to be in the subclasses Data, CalculationNode and WorkflowNode, because the tests currently, for simplicity, use base Nodes to create dummy graphs. If I were to put these checks in the Node base class, the type checks would fail. Until we rewrite the tests to only ever use Data, CalculationNode and WorkflowNode instances to create dummy graphs, this has to stay in the sub classes.

aiida/orm/node/process/workflow/workflow.py

muhrin · 2018-11-23T10:45:17Z

aiida/work/processes.py

+            # Need this special case for tests that use ProcessNodes as classes
+            if isinstance(self.calc, ProcessNode) and not isinstance(self.calc, (CalculationNode, WorkflowNode)):
+                self.calc.add_incoming(input_value, LinkType.INPUT_WORK, name)
+                continue


Suggested change

continue

The link triple is defined as the tuple of the source node, link type and link label of an incoming link into a target node. This concept is implemented in a named tuple `LinkTriple`. Since the introduction of link types, the link label of incoming links no longer necessarily needs to be unique, as long as the triple with the link type and source node *is* unique. However, the old implementation of the incoming link cache relied on label uniqueness, because it used only the label as the key in the cache, which used a dictionary as its data structure. Here we change that data structure of the node's internal incoming link cache to be a set of link triples.

The `link_label` used to be optional when specifying a link, as long as both nodes were already stored. If one of the nodes was unstored the link had to be stored in the internal cache which was indexed on the link label. This was already changed to be indexed on the link triple instead, solving that issue, but the auto label generation in `Node._add_dblink_from` in the case of no label being defined, relied on a uniqueness constraint of the link label on the database. This constraint had already been removed a long time ago when the `RETURN` link type was introduced. However, the logic of the auto label generation was kept, even though it would never work. We remove that functionality here and make the explicit passing of a link label required in the `add_incoming` and `_add_dblink_from` methods.

sphuber requested a review from giovannipizzi November 20, 2018 15:17

sphuber force-pushed the fix_2177_implement_new_link_types branch from bf4458c to c3ca6e4 Compare November 22, 2018 14:19

sphuber force-pushed the fix_2177_implement_new_link_types branch 3 times, most recently from 2de265a to 51ff0bc Compare November 22, 2018 19:20

Rename Node.add_link_from to Node.add_incoming

cec0d9f

This is to align it with the recent implementation of `get_incoming` and `get_outgoing` to obtain all incoming and outgoing links, respectively.

sphuber force-pushed the fix_2177_implement_new_link_types branch from 51ff0bc to 9db7997 Compare November 23, 2018 09:42

muhrin suggested changes Nov 23, 2018

View reviewed changes

sphuber force-pushed the fix_2177_implement_new_link_types branch 2 times, most recently from 58f8ed6 to ff8ba5b Compare November 23, 2018 18:01

sphuber force-pushed the fix_2177_implement_new_link_types branch 6 times, most recently from ddc18d0 to 1d94bad Compare November 24, 2018 19:07

sphuber force-pushed the fix_2177_implement_new_link_types branch from 1d94bad to 834e27a Compare November 24, 2018 23:55

muhrin approved these changes Nov 26, 2018

View reviewed changes

sphuber merged commit 3956299 into aiidateam:provenance_redesign Nov 26, 2018

sphuber deleted the fix_2177_implement_new_link_types branch November 26, 2018 17:45

giovannipizzi mentioned this pull request Dec 3, 2018

Transitive closure should not include return and call links! #298

Closed

sphuber mentioned this pull request Jan 11, 2019

Redesign of node hierarchy and link types #2152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new link types #2220

Implement new link types #2220

sphuber commented Nov 20, 2018 •

edited

Loading

coveralls commented Nov 20, 2018 •

edited

Loading

muhrin Nov 23, 2018

muhrin Nov 23, 2018

muhrin Nov 23, 2018

muhrin Nov 23, 2018

muhrin Nov 23, 2018

muhrin Nov 23, 2018

sphuber Nov 23, 2018

muhrin Nov 23, 2018

muhrin Nov 23, 2018

Implement new link types #2220

Implement new link types #2220

Conversation

sphuber commented Nov 20, 2018 • edited Loading

coveralls commented Nov 20, 2018 • edited Loading

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

sphuber commented Nov 20, 2018 •

edited

Loading

coveralls commented Nov 20, 2018 •

edited

Loading