Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export to dict #586

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions pyiron_workflow/nodes/composite.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from pyiron_snippets.colors import SeabornColors
from pyiron_snippets.dotdict import DotDict

from pyiron_workflow.channels import NOT_DATA, Channel
from pyiron_workflow.create import HasCreator
from pyiron_workflow.mixin.semantics import SemanticParent
from pyiron_workflow.node import Node
Expand All @@ -26,6 +27,130 @@
from pyiron_workflow.storage import StorageInterface


def _extract_data(item: Channel, with_values=True, with_default=True) -> dict:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type hint is wrong as Channel doesn't have value or default, and IMO this method should probably just live directly on the relevant class -- move it over to DataChannel?

data = {}
data_dict = {"default": NOT_DATA, "value": NOT_DATA, "type_hint": None}
if not with_values:
data_dict.pop("value")
if not with_default:
data_dict.pop("default")
for key, value in data_dict.items():
if getattr(item, key) is not value:
data[key] = getattr(item, key)
return data


def _is_internal_connection(channel: Channel, workflow: Composite, io_: str) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*sigh*. What I would like is all channels are only ever connected to other channels in the same scope. Unfortunately, nodes with no parent at all at least complicate this. Anyhow, this function isn't bad, it's just that it ought to be sufficient to merely use channel.connected. A cleaner solution would be to first go close #587, which I just opened because here you reminded me that we have this problem 😂 then come back here and not need this function at all.

"""
Check if a channel is connected to another channel in the same workflow.

Args:
channel (Channel): The channel to check.
workflow (Composite): The workflow to check whether the channel is connected to.
io_ (str): The IO direction to check.

Returns:
bool: Whether the channel is connected to another channel in the same workflow.
"""
if not channel.connected:
return False
return any(channel.connections[0] in getattr(n, io_) for n in workflow)


def _get_scoped_label(channel: Channel, io_: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _get_scoped_label(channel: Channel, io_: str) -> str:
def _scoped_label_to_io_label(channel: Channel, io_: str) -> str:

Function name lies!

return channel.scoped_label.replace("__", f".{io_}.")


def _io_to_dict(
node: Node, with_values: bool = True, with_default: bool = True
) -> dict:
data = {"inputs": {}, "outputs": {}}
is_composite = isinstance(node, Composite)
for io_ in ["inputs", "outputs"]:
for inp in getattr(node, io_):
if is_composite:
data[io_][inp.scoped_label] = _extract_data(
inp, with_values=with_values, with_default=with_default
)
else:
data[io_][inp.label] = _extract_data(
inp, with_values=with_values, with_default=with_default
)
return data


def export_node_to_dict(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. You hint Node, but not all Node have node_function. Hinting Function would be too narrowly scoped, as we do have this possibility for stuff like Transformer.

It is also not a good feeling to have export_node_to_dict in composite.py. The scoping is off. I guess we want to be able to convert nodes to dicts regardless of whether they're composite or not! So I think such tools need to either exist locally next to the thing they're converting, or be collected together in some third location like export.py as you mentioned elsewhere.

I also think that the composite's probably should also get a "node function", but it's something like Composite._on_run, or any other placeholder for the fact that we're going to execute the subgraph and how we'll do that.

Honestly, I think you will have a much easier time with this whole PR if you (or you wait for me eventually to) clean up the space you're working in -- i.e. if #504 is closed, we'll have a much cleaner division of "composite" vs "atomic" nodes right in the class hierarchy. #360 might help too, but it's harder and I think you can get away without it here.

node: Node, with_values: bool = True, with_default: bool = True
) -> dict:
"""
Export a node to a dictionary.

Args:
node (Node): The node to export.
with_values (bool): Whether to include the values of the channels in the
dictionary. (Default is True.)

Returns:
dict: The exported node as a dictionary.
"""
data = {"inputs": {}, "outputs": {}, "function": node.node_function}
data.update(_io_to_dict(node, with_values=with_values, with_default=with_default))
return data


def export_composite_to_dict(
workflow: Composite, with_values: bool = True, with_default: bool = True
) -> dict:
"""
Export a composite to a dictionary.

Args:
workflow (Composite): The composite to export.
with_values (bool): Whether to include the values of the channels in the
dictionary. (Default is True.)

Returns:
dict: The exported composite as a dictionary.
"""
data = {"inputs": {}, "outputs": {}, "nodes": {}, "edges": []}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All composite's are Nodes, yet we don't use export_node_to_dict as a starting point -- this makes me uncomfortable. An immediate result of this is that we wind up with the data dict being re-declared here and we're just relying on future maintainers to make sure all the string keys stay nicely synchronized.

for inp in workflow.inputs:
if inp.value_receiver is not None:
data["edges"].append(
(
f"inputs.{inp.scoped_label}",
_get_scoped_label(inp.value_receiver, "inputs"),
)
)
for node in workflow:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 100% correct, but it reminds me that I meant to open another issue... #588

We could proceed with such a definition now, but we'd need to remember to update it as part of that issue.

label = node.label
if isinstance(node, Composite):
data["nodes"][label] = export_composite_to_dict(
node, with_values=with_values
)
else:
data["nodes"][label] = export_node_to_dict(node, with_values=with_values)
for inp in node.inputs:
if _is_internal_connection(inp, workflow, "outputs"):
data["edges"].append(
(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still advocate for {} rather than ()

_get_scoped_label(inp.connections[0], "outputs"),
_get_scoped_label(inp, "inputs"),
)
)
for out in node.outputs:
if out.value_receiver is not None:
data["edges"].append(
(
_get_scoped_label(out, "outputs"),
f"outputs.{out.value_receiver.scoped_label}",
)
)
Comment on lines +146 to +153
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super similar to the same scraping for the input value receiver pairs, just a sort of conjugate. I think it should be possible to nicely pull out a _get_value_synchronization_edges(sender: Node, receiver: Node, io: str) function

data.update(
_io_to_dict(workflow, with_values=with_values, with_default=with_default)
)
return data


def _get_graph_as_dict(composite: Composite) -> dict:
if not isinstance(composite, Composite):
return composite
Expand Down Expand Up @@ -459,6 +584,13 @@ def graph_as_dict(self) -> dict:
"""
return _get_graph_as_dict(self)

def export_to_dict(
self, with_values: bool = True, with_default: bool = True
) -> dict:
return export_composite_to_dict(
self, with_values=with_values, with_default=with_default
)

def _get_connections_as_strings(
self, panel_getter: Callable
) -> list[tuple[tuple[str, str], tuple[str, str]]]:
Expand Down
44 changes: 44 additions & 0 deletions tests/unit/nodes/test_export.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import unittest

from pyiron_workflow._tests import ensure_tests_in_python_path
from pyiron_workflow.nodes.composite import Composite
from pyiron_workflow.workflow import Workflow

ensure_tests_in_python_path()


@Workflow.wrap.as_function_node
def add_one(a: int):
result = a + 1
return result

@Workflow.wrap.as_function_node
def add_two(b: int = 10) -> int:
result = b + 2
return result

@Workflow.wrap.as_macro_node
def add_three(macro: Composite | None = None, c: int = 0) -> int:
macro.one = add_one(a=c)
macro.two = add_two(b=macro.one)
w = macro.two
return w


class TestExport(unittest.TestCase):
def test_io_independence(self):
wf = Workflow("my_wf")
wf.three = add_three(c=1)
wf.four = add_one(a=wf.three)
wf.run()
data = wf.export_to_dict()
self.assertEqual(
set(data.keys()), {"edges", "inputs", "nodes", "outputs"}
)
self.assertEqual(
data["inputs"], {'three__c': {"default": 0, 'value': 1, 'type_hint': int}}
)


if __name__ == "__main__":
unittest.main()
Loading