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

Allow macro channel labels to be automatically truncated #84

Closed
liamhuber opened this issue Nov 20, 2023 · 4 comments
Closed

Allow macro channel labels to be automatically truncated #84

liamhuber opened this issue Nov 20, 2023 · 4 comments

Comments

@liamhuber
Copy link
Member

Ala #80. In the macro class, decorator, or both. For now I still lean towards the default using the full double underscore union of child and channel labels, and only truncating on explicit request, but I can at least imagine a world where the truncation is standard and we cleanly handle channel label conflict by falling back on the more verbose scheme.

@jan-janssen
Copy link
Member

Example code for discussion, initially I had:

@single_value_node("instance")
def get_instance(instance):
    return instance


@macro_node()
def internal_macro(wf: Macro) -> None:
    wf.get_instance = get_instance()
    wf.generate_structures = generate_structures(instance=wf.get_instance)
    wf.calc_with_calculator = calc_with_calculator(task_dict=wf.generate_structures)
    wf.fit = analyse_structures(
        instance=wf.get_instance, output_dict=wf.calc_with_calculator
    )
    wf.inputs_map = {
        "get_instance__instance": "instance",
        "calc_with_calculator__calculator": "calculator",
    }
    wf.outputs_map = {"fit__fit_dict": "fit_dict"}

I need the get_instance() function to connect one macro input to two function node inputs. Ideally, I would like to have something like:

@macro_node(output_labels="fit_dict")
def internal_macro(wf: Macro, instance, calculator) -> None:
    wf.generate_structures = generate_structures(instance=instance)
    wf.calc_with_calculator = calc_with_calculator(calculator=calculator, task_dict=wf.generate_structures)
    wf.fit = analyse_structures(
        instance=instance, output_dict=wf.calc_with_calculator
    )
    return wf.fit.fit_dict

With this syntax the user has all the flexibility which nodes to make available on the outside and how to link them internally. Using inspect.get_signature() it should be possible to get the input arguments, assign them to the workflow and then return the variables as links to the workflow nodes again, like:

wf.instance = instance 
instance = wf.instance 

And the output could be handled in analogy to the node definitions.

@liamhuber
Copy link
Member Author

This is a separate and less clean issue, I'm opening something fresh for it so this one can still be closed once the automatic truncation gets implemented.

@liamhuber
Copy link
Member Author

@jan-janssen I think what you're asking for is now handled in #134.

The actual title here, about truncating macro/workflow labels from some_node__some_channel straight to some_channel always came with the downside of being implicit. It is still possible, and when conflicts arise we'd have to automatically revert back to the fully scoped label. However, now that Macro has the nice as-a-function definition possible, this truncation loses a lot of utility and is really only helpful for Workflow instances.

However, there I think it may do more harm than good. Workflows are intended to be highly dynamic -- if it's set in stone what you want to do, you might as well make a macro. To support this, workflows dynamically produce their IO on each access call based on user defined maps (if any) and unconnected IO. So what I can imagine happening all the time if we're truncating (i.e. de-scoping) workflow IO labels, is that a user has some node with (in)output foo, and they go around doing things like wf(..., foo=42, ...). Then later they add another copy of that node to the workflow (or literally any other node who's (in)output is named "foo"), and all of a sudden calls like wf(..., foo=42, ...) magically stop working because they suddenly need wf(..., node1__foo=42, node2__foo=None,...) to avoid name conflicts.

I'm super happy with #134, but at this point I just don't see automatic and implicit label truncation being a net win. I'm open to counter-arguments, but if I've convinced you then let's close this issue.

@liamhuber
Copy link
Member Author

Per discussion in meeting just now, Jan and I are both happy to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants