pyiron / pyiron_workflow

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

[patch] Refactor init and post #292

Closed liamhuber closed 2 months ago

liamhuber commented 2 months ago

This eliminates the metaclass HasPost from Node; previously this was used to ensure that loading and running nodes always happened after everything in __init__ was totally finished. This was important because Macro (currently...) needs to have built its subgraph before some storage can load its child data, and because obviously we want all the children of Node to be finished their setup procedures before we try running.

In its place, Node.__init__ now ends with self._setup_node(); self._after_node_setup( *args, overwrite_save=overwrite_save, run_after_init=run_after_init, **kwargs). Children can thus (a) do stuff before calling super().__init__ and (b) override _setup_node in order to get everything done that needs to be done before loading and running in Node._after_node_setup.

This places a bit more cognitive load on developers of Node subclasses, as they need to remember the business with _setup_node when defining the subclass, but it relieves the complication of using a metaclass. I discussed the idea with @pmrv and he agreed this is probably the lesser of two evils, saying something to the effect of "this way it is at least much clearer when we are going to shoot ourselves in the foot". This is definitely a trade-off with downside, but I think it's a net win.

Note that typical users/developers who are creating new macro, function, and workflow nodes, e.g. with the decorators, don't give a hoot about any of this. They can keep on decorating functions and instantiating workflows to their hearts content. It is only development at one stage above these "node designers" that need to know about/care about this stuff.

It's also the case that Node and its children now parse all remaining **kwargs and *args as node input channel data, just like Function always did! IMO this is nice since Macro definitions are now very function-like and users/developers are often likely to have a good sense of the order of arguments, so allowing *arg specification should be convenient. This is also the case for parsing Node.run and all the run-like methods. The one exception is Workflow.__init__, where *nodes takes the place of *args and these are considered initial children. Anyhow, this is purely an extension/relaxation of the existing syntax.

review-notebook-app[bot] commented 2 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

github-actions[bot] commented 2 months ago

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

codacy-production[bot] commented 2 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: +0.04% (target: -1.00%) :white_check_mark: 100.00%
Coverage variation details | | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (f4fdaa702c5199b546f0f3726280c701eafcf34f) | 3507 | 3074 | 87.65% | | | Head commit (d7fc9c4d369234c777be298f9bc518c835248a3f) | 3501 (-6) | 3070 (-4) | 87.69% (**+0.04%**) | **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 (#292) | 46 | 46 | **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

Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more