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

Change of syntax WorkGraph.add_link #176

Open
khsrali opened this issue Jul 17, 2024 · 3 comments
Open

Change of syntax WorkGraph.add_link #176

khsrali opened this issue Jul 17, 2024 · 3 comments

Comments

@khsrali
Copy link
Contributor

khsrali commented Jul 17, 2024

I found this syntax a bit difficult to digests:

WorkGraph.add_link(wg.tasks["add1"].outputs["result"], wg.tasks["multiply1"].inputs["x"])

Since input and output are the primary keywords for add_link, how about a syntax like this:

WorkGraph.add_link(input= "add1", output="multiply1")

Intuitively every link is from output of a node to input of another node.

@superstar54
Copy link
Member

Thanks for the suggestions. Since we need both the names of the task and the socket (port), I assume you mean:

WorkGraph.add_link(input= "add1.result", output="multiply1.x")

Or more simple:

WorkGraph.add_link("add1.result", "multiply1.x")

Or more intuitively:

WorkGraph.add_link(from_task= "add1", from_socket="result", to_task="multiply1", to_socket="x")

In principle, we can provide these options to simplify the syntax; which one do you suggest?

@GeigerJ2
Copy link
Contributor

Just a note here that the add_task method actually returns the task. So one could assign this task to a variable and then access it directly, rather than having to use wg.tasks["task_name"] in the link. This would at least simplify @khsrali's code snippet to:

wg.add_link(add_task.outputs["result"], multiply_task.inputs["x"])

given that both tasks were previously added to the WorkGraph, e.g.:

wg = WorkGraph()
add_task = wg.add_task(add, name="add1", x=<foo>)
multiply_task = wg.add_task(multiply, name="multiply1")
wg.add_link(add_task.outputs["result"], multiply_task.inputs["x"])

Though, it would be nice to have tab completion. This works for add_task., where tab shows outputs. However, it doesn't then work for add_task.outputs., to show result. Probably this is due to the use of kwargs in the WorkGraphInputSocketCollection and I don't know if there is any way to make this possible (though, add_task.outputs.result also works to access the result socket).[^1]

Now, regarding @superstar54's response: From our discussion this morning, it seems like overall, we would like to move away from the use of these magic strings, so I would vouch against the provided options, as users don't know what is possible. If the identifiers are still attached to the instances, it's at least easier to inspect them. Also, I just saw that internally add_link uses source and target, so, in principle, the first and second option (apart from the string identifier ofc) are already possible anyway, and here the names are just changed from source/target to input/output.

And lastly, I'm not sure about the third option. The [from,to]_task and [from,to]_socket are very explicit which could be good, but it introduces WorkGraph lingo, and might be a bit excessive to write. At this point, I don't really see the need to change the current API, to be honest.

PS:
Also, is there any difference between:

add_task = wg.add_task(add, name="add1", x=<foo>)
multiply_task = wg.add_task(multiply, name="multiply1")
wg.add_link(add_task.outputs["result"], multiply_task.inputs["x"])

and

add_task = wg.add_task(add, name="add1", x=<foo>)
multiply_task = wg.add_task(multiply, name="multiply1", x=add_task.outputs["result"])

I suppose not, they should behave exactly the same? I think it's nice that WorkGraph provides multiple ways of achieving the same thing for different use cases - maybe someone wants to add the link directly with the task, maybe someone wants to add it later - we just need to be careful that this doesn't cause confusion to the users (and in this case, I think it's preferred to have both).

@khsrali
Copy link
Contributor Author

khsrali commented Jul 17, 2024

After discussing with @GeigerJ2 actually what I propose is to use and advertise the standard object approach, like this:

This way you have:

WorkGraph.add_link(source= add_1.output['result'], target=multiply_1.input['x'])
and /or
WorkGraph.add_link(source= add_1.result, target=multiply_1.x)

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

No branches or pull requests

3 participants