(WIP) Removing graph edge causes disconnection in underlying model

[liamhuber]

Closes #45

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[liamhuber]

This works ok when disconnecting edges in a flat graph, or between a node and a group when the group is the upstream partner, but when the group is the downstream partner it still never actually disconnects the underlying node. This is evidenced by the last two assertions in 3712983

I'm running out of time to actually solve this today, so I want to document my current best guess of where the problem is:

• base.get_updated_graph encounters the "group" node that is node expanded, and calls base.collapse_node on it
• base.collapse_node finds the "graph"-type node, and invokes base.remove_hidden_nodes and then base._remove_edges_to_hidden_nodes
• base._remove_edges_to_hidden_nodes doesn't do anything to the underlying simple_workflow Nodes
• I believe disconnecting the underlying simple_workflow Nodes here would resolve the proximate problem, but we can't naively leverage the new _disconnect_receiving_port functionality here, because we've already removed the nodes from the graph at this point and have lost our reference to the underlying Nodes -- unless the order/composition of operations is adjusted, we're left playing with only the edge list

I believe this bug is a symptom of the complexity that comes with maintaining two separate workflow representations in simple_workflow and graph.

Tests so far do not cover any cases where a group is in the expanded state.

[JNmpi]

• Could we enforce a collapse_node (if it is a macro in expanded state) before removing the edge? This should avoid the scenario you described above? This is not the full solution we should aim at but should provide a robust route to handle this case.
• Do we need if edge.sourceHandle == "self": raise NotImplementedError()? Shouldn't the same approach used for non-node input work also for node input?

[liamhuber]

• Could we enforce a collapse_node (if it is a macro in expanded state) before removing the edge? This should avoid the scenario you described above? This is not the full solution we should aim at but should provide a robust route to handle this case.

I don't recommend going this route. It can't be done naively, because otherwise we would have users sitting in the GUI, disconnecting a node, and suddenly and unexpectedly having their expanded node collapse on them. So at a minimum, we need to check on disconnect whether the collapse is needed, log whether the collapse has been done, and then reverse it at the end. Overall, I still have a poor grasp of the side effects of the expanded flag; we would need, at this point, the inverse of collapse, which is uncollapse not expand, yet we're triggering off the expanded key.

I actually already tried locally to follow my attack pattern outlined above -- copy-pasting remove_hidden_node and _remove_edges_to_hidden_nodes directly into collapse_node and then adjusting the order of the steps -- but this didn't work. I still need to figure out if it didn't work because I implemented it poorly, or if it didn't work because it's a bad plan. Above all, what I'm trying to avoid here is introducing new bugs we don't know about just to solve this one we are aware of.

• Do we need if edge.sourceHandle == "self": raise NotImplementedError()? Shouldn't the same approach used for non-node input work also for node input?

Quite possibly. I haven't dug into how you implemented the "self" connection yet, and so to avoid introducing new bugs I'm not aware of, I just fail hard on this branch.

Overall, I'm trying to introduce symmetries, invariances, and transitivities to the model -- uncollapse as the opposite of collapse, remove_edge undoing add_edge (in both model layers), etc. So as a starting point I looked at add_edge and saw that on detecting subgraph interaction (i.e. talking to a virtual node), the sub-method called does nothing when self is involved, with a comment that something still needs to be implemented:

pyiron_core/pyiron_core/pyiron_workflow/graph/base.py

Lines 370 to 379 in 65089ff

	if not (is_virtual(source) or is_virtual(target)):
	new_graph = _update_target_port(new_graph, new_graph.edges[-1])
	return new_graph
	def _update_target_port(graph: Graph, edge: GraphEdge):
	if edge.sourceHandle == "self":
	print("implement connect to self")
	pass
	else:

Here, calling _disconnect_receiving_port in remove_edge is analogous to _update_target_port in add_edge, and so I was alerted that I may need special behaviour here too. This way, I'll know if this path is missing from the tests, can cook up an example that forces it to happen, and then can proactively make sure it's behaving in a desirable way.

[JNmpi]

I fully support your approach of introducing symmetries, invariances, and transitivities to these operations. It nicely aligns with what I did for the GUI interface, which calls for these operations and expects symmetries, etc. It will also be beneficial to incorporate them into the unit tests. This will allow us to later test more robust schemes and approaches that incorporate these principles directly.

[liamhuber]

I still haven't figured this out, but I made some progress.

The problem doesn't come from simply adding and removing the edge, but only if you perform other operations while the edge is there:

from pyiron_core.pyiron_workflow.graph import (
    base,
    edges,
    group,
    run,
)

g = base.Graph(label="virtual_target")
g = base.add_node(g, base.identity(label="n1", x=0))
g = base.add_node(g, base.identity(label="n2"))
g = base.add_node(g, base.identity(label="n3"))
g = base.add_edge(g, "n2", "n3", "x", "x")
g = group.create_group(g, ["n2", "n3"], label="group")

virtual_edge = edges.GraphEdge("n1", "va_i_group__n2__x", "x", "x")
g = base.add_edge(
    g,
    virtual_edge.source,
    virtual_edge.target,
    virtual_edge.sourceHandle,
    virtual_edge.targetHandle,
)

# intermediate = base.get_updated_graph(g)  # Causes later pull to return None
# run.pull_node(intermediate, "group")  # Causes later pull to return 0

g = base.remove_edge(g, virtual_edge)

gu = base.get_updated_graph(g)
assert(len(gu.edges) == 0)
try:
    out = run.pull_node(gu, "group")
    print(f"Run ought to crash, but instead got: {out}")
except ValueError as e:
    assert("Input data missing" in str(e))
    print("This is what we want -- with no edge, running should fail")

So something in these two operations is impacting the graph and/or underlying workflow model(s) more deeply than the copying in gu = base.get_updated_graph(g) can handle; underlying data referenced by g is getting mutated.