-
Notifications
You must be signed in to change notification settings - Fork 5
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
Create subsection "Nested workflow" in the docs #277
base: main
Are you sure you want to change the base?
Conversation
94e0a29
to
b4ed841
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
==========================================
+ Coverage 75.75% 80.60% +4.85%
==========================================
Files 70 66 -4
Lines 4615 5135 +520
==========================================
+ Hits 3496 4139 +643
+ Misses 1119 996 -123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
16e2070
to
b059def
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the if
and while
task again, because we now have new If
task and while
task, thus there are two ways to implement if
if
task, dose not need nested workgraph- use
graph_builder
, thus nested workgraph.
thus it's better to keep if
and while
separately, and not move to the nested_workgraph
.
I suggest at the end of the graph_builder.py
, we make a link to show the application in the if
and while
.
fb9410e
to
c344a45
Compare
I dont think the graph_builder example is really about the dynamic part as it just forwards inputs like any task does it. We could make another example that runs different tasks depending on the inputs. Something like @task.calcfunction()
def add_one(x):
return x.value+1
@task.calcfunction()
def modulo_five(x):
return x % 5
@graph_builder(outputs = [...])
def my_modular(i: orm.Int):
wg = WorkGraph()
if i.value < 5:
task = wg.add_task(add_one)
else:
task = wg.add_task(modulo_five)
# need wg.selector to expose for linking?
return wg Then one could say that this does not preserve provenance and directly interlude to the |
c344a45
to
7f4145c
Compare
Added an example for a dynamic use case of the graph_builder |
a57e316
to
b3d0cad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @agoscinski , Thanks for the work.
I would suggest not creating a dynamic-workflows
section, but moving all three notebooks into the howto
, so that user can see the dynamic
, if
and while
immediately.
@task.graph_builder(outputs=[{"name": "result", "from": "context.out"}]) | ||
def add_modulo(i: Int): | ||
wg = WorkGraph() | ||
if i.value < 2: | ||
task = wg.add_task(add_one, x=i) | ||
else: | ||
task = wg.add_task(modulo_two, x=i) | ||
|
||
task.set_context({"result": "out"}) | ||
return wg | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplicate as in the if.
I would suggest using a for loop inside the graph_builder so that the number of tasks depends on an input value, making the work graph dynamic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have quite a few for loops in the examples too therefore I don't see duplication as a reason to rather use for loops. I feel this example is more educative because you are changing the type of task, you are in different branch of your program depending on your input which might be more intuitive to be dynamic.
But I can also just add both.
Why ti does not preserve provenance? |
Okay, but the howto's starting to get messy and need some structure at some point, but I think for now one still can put everything there |
I mean more that it does not store provenance as transparently as using the |
I merged now the nested and dynamic example because it seemed strange to see them separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @agoscinski , thanks for the update!
The dynamic_graph_builder.py
is not used, I think you want to remove it.
# Nested workflows | ||
# ================ | ||
# The `Graph Builder` allow user to create nested workflows from an input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to give an overview: user can create nested WorkGraph in two ways:
- Create a Task from the workgraph
- Use graph builder.
Also discuss what's the upside and downside of the two approaches, or later in each section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have done something similar
# For that use case we need to use the graph builder | ||
|
||
# Create a graph builder function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this empty line, otherwise the format will be wrong.
# Suppose we want a WorkGraph which includes another WorkGraph`(x+y)*z` inside it. | ||
# We can actually add a WorkGraph to another WorkGraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs to the Create a Task from the workgraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is at the beginning now
# However linking the two WorkGraphs will not work | ||
|
||
wg = WorkGraph("nested_workgraph") | ||
add_multiply1 = wg.add_task(add_multiply(x=Int(2), y=Int(3), z=Int(4))) | ||
|
||
try: | ||
wg.add_task( | ||
add_multiply(x=add_multiply1.outputs["multiply.result"], y=Int(3), z=Int(4)) | ||
) | ||
except Exception as err: | ||
print(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one can link the two WorkGraph tasks. But we need to write the code in a different way:
def add_multiply(x=None, y=None, z=None):
wg = WorkGraph()
wg.add_task(add, name="add", x=x, y=y)
wg.add_task(multiply, name="multiply", x=z)
wg.add_link(wg.tasks["add"].outputs[0], wg.tasks["multiply"].inputs["y"])
return wg
wg = WorkGraph("nested_workgraph")
add_multiply1 = wg.add_task(add_multiply(x=2, y=3, z=4), name="add_multiply1")
add_multiply2 = wg.add_task(add_multiply(y=5, z=6), name="add_multiply2")
wg.add_link(add_multiply1.outputs["multiply.result"], add_multiply2.inputs["add.x"])
wg.run()
The difference between the above code and a graph_builder
task is that in graph_builder
, the workgraph is dynamic and only created during execution, while the above workgraph is static, so that we can access the socket directly, e.g., add.x
.
Both can used to create nested workgrpah, but graph_builder
is a black box, which is the downside. The upside is that it allows dynamic workgraph generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay used that example and explained a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. it is not properly working
# define add task
@task.calcfunction()
def add(x, y):
return x + y
# define multiply task
@task.calcfunction()
def multiply(x, y):
return x * y
def add_multiply(x=None, y=None, z=None):
wg = WorkGraph()
wg.add_task(add, name="add", x=x, y=y)
wg.add_task(multiply, name="multiply", x=z)
wg.add_link(wg.tasks["add"].outputs[0], wg.tasks["multiply"].inputs["y"])
return wg
wg = WorkGraph("nested_workgraph")
# Creating a task from the WorkGraph
add_multiply1 = wg.add_task(add_multiply(x=Int(2), y=Int(3), z=Int(4)))
add_multiply2 = wg.add_task(add_multiply(x=Int(2), y=Int(3)))
# link the output of a task to the input of another task
wg.add_link(add_multiply1.outputs[0], add_multiply2.inputs["multiply.x"])
wg.to_html()
# %%
# Run the workgraph
wg.run()
Gives
Error: Error in task multiply: Cannot convert value of type <class 'aiida.orm.utils.managers.NodeLinksManager'> to AiiDA type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay now it works!
# Create a Task from the workgraph (Experimental) | ||
# ----------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you mentioned this part at the beginning, so better to move this section before graph_builder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is now integrated into the first part
Moves the if task and while task examples into this subsection.
8e823f3
to
fe738a0
Compare
Co-authored-by: Xing Wang <xingwang1991@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Moves the graph builder, if task and while task examples into this subsection. Also removed the graph_builder.ipynb since it is now generated. Solves issue #195