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

Node input as args #769

Merged
merged 38 commits into from
Aug 2, 2023
Merged

Node input as args #769

merged 38 commits into from
Aug 2, 2023

Conversation

liamhuber
Copy link
Member

So far, this modifies Function.__call__ such that it updates the input channels using **kwargs and/or *args. This is the first step towards being able to instantiate nodes (at least those created by a decorator) with their initial input values set as args, as requested by @JNmpi in #756.

@liamhuber liamhuber added the .workflow Pertaining to the workflow submodule label Jul 12, 2023
@liamhuber liamhuber marked this pull request as draft July 12, 2023 23:08
@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_contrib/node_input_as_args

@github-actions
Copy link
Contributor

Pull Request Test Coverage Report for Build 5537284624

  • 24 of 24 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 13.042%

Totals Coverage Status
Change from base Build 5536934835: 0.1%
Covered Lines: 1888
Relevant Lines: 14476

💛 - Coveralls

@liamhuber
Copy link
Member Author

I let this idea percolate overnight, and I think the issue is that in comparison to Joerg's demo in pyiron/pyiron_workflow#14, our Function(Node) instances have their own important Node kwargs in addition to the arguments of the underlying function being wrapped. So in both instantiation and at call time we need to parse out which kwargs are being used to update node properties, and which are being used to update input channels.

Of course, no such division of *args is sensibly possible -- if the wrapped function accepts strings as output, how could we ever know if a particular arg is meant as an output channel label or input data? For this reason, before working further on this PR, I'd like to go back and implement @samwaseda's idea to automatically parse output labels in #717. Of course we can still allow custom output labels, but they'd be from a kwargs output_labels: Optional[tuple | list] = None instead of the args *output_labels. Then we can rebase this PR on top of that and work in a world where *args are always interpreted as input data... at least for Function(Node), since for Composite(Node) there is no explicit order to the input so we'll still need to do input data updates with kwargs.

@liamhuber liamhuber mentioned this pull request Jul 13, 2023
3 tasks
@JNmpi
Copy link

JNmpi commented Jul 15, 2023

Thanks, @liamhuber for your thoughts. I am not sure that I understand the issue of parameters coming from the function we want to wrap and those related to the node. The strings needed for the output label are arguments in the decorator, whereas all function arguments are contained in the function definition. Thus, when converting the function via a decorator the two sets of parameters should be straightforward to distinguish. Or is there something I am missing? It may be helpful to give a code example.

@liamhuber
Copy link
Member Author

Hi @JNmpi, the problem was that the strings for the output labels were provided to the decorator and/or the underlying node class at instantiation. Since there's no robust way to distinguish whether an *arg is intended as a label (arg of the class) or function input (arg of the function the class is wrapping), so we were really stuck. Now that output labels are automatically parsed or provided as a kwarg, *args are always function input and we're a-ok.

The other comment was just that we want to be able to pass, eg, run_on_updates=True at instantiation (and on call?) and recognize that this is a node class kwarg and not wrapped function input. This is not a problem at all and we already have all the tools to handle it; I should be able to get it put together on Monday.

@liamhuber liamhuber changed the base branch from main to parse_output_labels July 17, 2023 16:49
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@liamhuber liamhuber changed the title WIP: Node input as args Node input as args Jul 17, 2023
@liamhuber liamhuber requested a review from samwaseda July 17, 2023 20:47
@liamhuber liamhuber marked this pull request as ready for review July 17, 2023 20:47
@liamhuber
Copy link
Member Author

liamhuber commented Jul 17, 2023

@samwaseda, @JNmpi, there is of course more that can and will be added, but #770 and this PR together make something I'm pretty happy with. We now have a lot more freedom in terms of (not) labelling output channels, and updating input at init and on call with both args and kwargs. Example:

from pyiron_contrib.workflow import Workflow

wf = Workflow("simple")

@Workflow.wrap_as.single_value_node()
def add_one(x):
    y = x + 1
    return y  # Output labels parsed automatically

@Workflow.wrap_as.function_node(output_labels="sum")  # Output labels optionally overridden
def adder(x: int | float = 1, y: int | float = 1) -> int | float:
    return x + y

# Function node input data as args, kwargs, or both!
wf.a = add_one(0)   # arg
wf.b = add_one(x=0)   # kwarg
wf.sum = adder(wf.a, y=wf.b)   # both
# Remember, with single value nodes we can pass the whole node instead of an output channel!

# Workflow automatically generate {node.label}_{channel.label} IO
print(wf.outputs.sum_sum.value)
>>> 2

# Can call a workflow, but since input has no order _only_ kwargs work
wf(a_x=2, b_x=3)
# The nodes are set to all update automatically, 
# so this will propagate through the workflow without any wf.run() invokation
print(wf.outputs.sum_sum.value)
>>> 7

Docs, tests, and example notebook are all updated accordingly.

@samwaseda
Copy link
Member

Sorry I cannot review until Wednesday evening!!

@liamhuber
Copy link
Member Author

Sorry I cannot review until Wednesday evening!!

No worries! I've got extra childcare stuff today anyways; if I do manage to get any more done I'll just keep stacking it on top.

liamhuber added 17 commits July 18, 2023 11:17
Namely, on_run should be a property returning a callable
They are not implemented and working yet, so at least fail cleanly!
And downstream stuff like `update` and thus `__call__`. This was requested by Joerg and now makes things really start to feel like regular python
We may wish to later make Macro's slow, but for Workflows, since the IO is just routing through to the owned nodes, input updates are _anyhow_ most of the time re-running things, so it's a sensible default IMO
Return output when calling `run`
@liamhuber liamhuber merged commit f0434bd into parse_output_labels Aug 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the node_input_as_args branch August 2, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.workflow Pertaining to the workflow submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants