pyiron / pyiron_workflow

Graph-and-node based workflows
BSD 3-Clause "New" or "Revised" License
10 stars 1 forks source link

[minor] Function selflessly #279

Closed liamhuber closed 2 months ago

liamhuber commented 2 months ago

Currently, we jump through some hoops to let users pass self as a special argument for Function nodes. This didn't show up in the node input, and under the hood the node instance got passed in for that argument. You could do things like this:

from pyiron_workflow import Workflow

@Workflow.wrap_as.function_node()
def Selfish(self, x):
    self.some_attribute = 5
    return x

n = Selfish()
print(n.inputs.labels)
>>> ["x"]

n(x=42)
print(n.some_attribute)
>>> 5

Now we don't, and self is treated just like any other Function node argument, which means it bumps right into the protection we have in place to stop people from using terms already in the Function.__init__ signature (this protection allows input to be set as **kwargs)

from pyiron_workflow import Workflow

@Workflow.wrap.as_function_node()
def Selfish(self, x):
    self.some_attribute = 5
    return x

n = Selfish()
print(n.inputs.labels)
>>> ValueError: The Input channel name self is not valid. Please choose a name _not_ among ['self', 'args', 'label', 'parent', 'overwrite_save', 'run_after_init', 'storage_backend', 'save_after_run', 'kwargs']

I like for a bunch of reasons:

I.e. if you want state, use a state-ful Macro node and hold the state as IO of one of its children.

Non-goals:

EDIT: python syntax highlighting on the examples

github-actions[bot] commented 2 months ago

Binder :point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/functionselflessly

codacy-production[bot] commented 2 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: +0.03% (target: -1.00%) :white_check_mark: 100.00%
Coverage variation details | | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (0f33bec9c0a9ce3f40820b3b1b15847f3c4806be) | 3447 | 3003 | 87.12% | | | Head commit (61eac44116687614a5093da3ca1fcdc48ce65052) | 3431 (-16) | 2990 (-13) | 87.15% (**+0.03%**) | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `
Diff coverage details | | Coverable lines | Covered lines | Diff coverage | | ------------- | ------------- | ------------- | ------------- | | Pull request (#279) | 4 | 4 | **100.00%** | **Diff coverage** is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: `/ * 100%`

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

samwaseda commented 2 months ago

Sorry I had a heated discussion yesterday elsewhere and could not join the pyiron meeting. I was never very comfortable with self (I guess same for you) so I'm ok with getting rid of it. But this only removes self right? I thought the intention of self was to pass information from outside, like:

from pyiron_workflow import Workflow

@Workflow.wrap.as_function_node()
def Selfish(self, some_executable):
    some_executable.cores = self.server.cores
    some_executable.run()
    return some_executable.output

Or do we now want to do everything via function arguments?

liamhuber commented 2 months ago

Ah, I had only been thinking in terms of adding state to the node rather than pulling data from existing state. At any rate, I agree we can probably manage to do it via the function input. E.g.

from pyiron_workflow import Workflow

@Workflow.wrap.as_function_node()
def Selfless(some_executable, cores):
    some_executable.cores = cores
    some_executable.run()
    return some_executable.output

n = Selfless()
n(some_executable=exe, cores=n.server.cores)

This is not identical functionality, but let's run with it and add back in self only in the dire case that it's unavoidably necessary for a particular application.

EDIT: python highlighting