[minor] Make `Function` and `Macro` definition functions available at the class level

liamhuber commented 3 months ago

I'm still hacking away at getting Joerg's iter functionality and node library integrated into the codebase in a robust, storage-compatible way. Most of the attacks I've thought of involve knowing about the IO (labels and hints) at the class level, so I stopped and came back to #190, which wants to make Function.node_function and Macro.graph_creator both class methods.

The attack I'm taking is to make the existing Macro and Function calls actually be functions that dynamically create and return children of AbstractMacro and AbstractFunction (formerly the Macro and Function classes, respectively) by invoking the decorator. This approach made the refactor for Function super duper easy and I'm quite happy with it so far.

TODO:

[x] Repeat the attack for Macro
[x] Update docs ~and notebooks~

Closes #190

Non-goals:

Actually doing anything with this class method (like exploiting it to expose default IO channel patterns or similar) -- this is a fine idea but a separate PR.
Discussing this in the tutorial notebooks -- we want people using the decorators anyhow

Minor bump because any nodes inheriting from Function/Macro now need to inherit from AbstractFunction/AbstractMacro (afaik there are no such nodes outside this repo, but better to be safe).

github-actions[bot] commented 3 months ago

:point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/io_info_asclassmethod

codacy-production[bot] commented 3 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
:white_check_mark: +0.05% (target: -1.00%)	:white_check_mark: 88.89%

Coverage variation details

| | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (e40e2a88e5b9d04f0f72df4d23e0f15b1d00d62f) | 3388 | 2969 | 87.63% | | | Head commit (52497d602cb374048145c9c54d93412f7839a66e) | 3411 (+23) | 2991 (+22) | 87.69% (**+0.05%**) | **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 (#265) | 54 | 48 | **88.89%** | **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}

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 8543228585

Details

84 of 85 (98.82%) changed or added relevant lines in 6 files are covered.
58 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.06%) to 91.904%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_workflow/composite.py	2	3	66.67%
<!--	Total:	84	85	98.82%	-->

Files with Coverage Reduction	New Missed Lines	%
function.py	11	92.72%
macro.py	12	90.0%
create.py	17	87.8%
composite.py	18	89.69%
<!--	Total:	58	-->

Totals
Change from base Build 8543181225:	0.06%
Covered Lines:	6414
Relevant Lines:	6979

💛 - Coveralls

liamhuber commented 3 months ago

@pmrv I thought you might find this somewhat interesting given our discussion in #190. I wound up leveraging __new__ after all, but instead of doing it in the real inheritance tree, it's happening in a little shell class that doesn't do much other than dynamically create the new class! Basically these are just class-like alternatives to the decorator functions!

pmrv commented 2 months ago

I'm not sure I follow all the call sites, but if func = Function(...) just makes a new dynamic class and not isinstance(func, Function) holds, wouldn't it be simpler and less indirect to just have Function as a plain function or method on the creator where it will end up being called from?

liamhuber commented 2 months ago

@pmrv, yes, honestly this was probably an instance of "Your Scientists Were So Preoccupied With Whether Or Not They Could, They Didn't Stop To Think If They Should". There is some rationale to keep the callable PascalCase, since in this frameworks that indicates to our users that they're getting a Node instance back, and a big block of our target audience is people who aren't even aware of OOP. So making it a class has the advantage that my IDE doesn't complain to me that I'm casing a function wrong 😂 But you're absolutely right, while there's no operational difference between a function returning the new instance and a class that only overrides __new__ and returns the same new instance, the direct function is easier to maintain.

I'll try switching it back to a function later today. I don't anticipate any complications, but if any arise I'll bail on this and just delay it until the planned refactoring where I extract a bunch of shared behaviour between AbstractFunction and AbstractMacro into a mutual parent.

liamhuber commented 2 months ago

@pmrv right, ok, I remember now why I did it this way. For Function it absolutely could just be an actual function and not a class. But for Macro, AbstractMacro (and Workflow, and everything that inherits from Composite) has access to the creator at the class-level. Since Function and Macro look like node classes, I wanted to be able to offer the same creator access for consistency. E.g. one could, in principle, write

from pyiron_workflow.macro import Macro

def whatever(macro, a, b):
    macro.add = Macro.create.standard.Add(a, b)
    return macro.add

m = Macro(whatever, output_labels="sum")

I'm still open to renaming Function->function and Macro->macro so they look like functions again and then get rid of this feature, but because of all the places Function and Macro get called that will almost certainly wind up causing inane merge conflicts as I try to pull the change up through the PRs stacked on top of this one...so if we're going to do it, let's do it later.

codacy-production[bot] commented 2 months ago