-
Notifications
You must be signed in to change notification settings - Fork 0
support caching of inputs from non-required links #162
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
Changes from all commits
306e110
5923719
aedb436
4320dab
a85709c
c50c41e
2e56f29
a590ae6
a364168
93fcb74
fae606c
d4025fe
de5320e
c72f6a9
d3749a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,8 +99,10 @@ def submodel21_on_error(): | |
| def workflow21(on_error): | ||
| if on_error: | ||
| submodel21 = submodel21_on_error | ||
| out1_required = False | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI this test fails in the main branch when flowchart TD
n1["in"]
n2["raise_not_greater_than"]
n3["out1"]
n4["out2"]
n5["out"]
n1 --> n2
n2 --> n3
n2 -->|on error| n4
n3 -->|"required=False" fixes the test| n5
n4 --> n5
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Graph analysis before Graph analysis since With
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before ewokscore 5.0.0rc1 def link_is_required(
graph: networkx.DiGraph, source_id: NodeIdType, target_id: NodeIdType
) -> bool:
if link_has_required(graph, source_id, target_id):
return True
if link_is_conditional(graph, source_id, target_id):
return False
not_required = node_has_ancestors(
graph, source_id, link_has_required=False, link_is_conditional=True
)
not_required |= node_has_ancestors(graph, source_id, node_has_error_handlers=True)
return not not_requiredSince ewokscore 5.0.0rc1 def link_is_required(
graph: networkx.DiGraph, source_id: NodeIdType, target_id: NodeIdType
) -> bool:
# Explicitly required or optional
if link_is_explicitly_required(graph, source_id, target_id):
return True
if link_is_explicitly_optional(graph, source_id, target_id):
return False
# By default, conditional links are optional
if link_is_conditional(graph, source_id, target_id):
return False
# By default, links with at least one non-required link upstream become non-required
return not node_has_ancestors(graph, source_id, link_is_required=False)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition makes the difference (which is now removed since 5.0.0rc1) optional = node_has_ancestors(graph, source_id, node_has_error_handlers=True)It makes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no right or wrong answer here. We just need to be sure the ewoks SPEC is clear. New SPEC of the link attribute `requiredhttps://ewokscore.readthedocs.io/en/latest/reference/specs.html#link-attributes
Old SPEC of the link attribute `requiredhttps://ewokscore.readthedocs.io/en/v4.0.2/reference/specs.html#link-attributes
ConclusionNote that the old description does not actually describe the way the graph analysis considers ancestors to be required when So
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So before flowchart TD
n1["in"]
n2["raise_not_greater_than"]
n3["out1"]
n4["out2"]
n5["out"]
n1 --> n2
n2 --> n3
n2 -->|on error| n4
n3 -->|"required=False" from old analysis which is desired| n5
n4 --> n5
Before flowchart TD
n1["in1"]
n2["raise_not_greater_than"]
n3["out1"]
n4["out2"]
n5["out3"]
n6["out4"]
n7["needed input"]
n1 --> n2
n2 --> n3
n2 -->|on error| n4
n3 -->|"required=False" from old analysis which is NOT desired| n5
n4 --> n6
n1 --> n7
n7 --> n5
When
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course the long term goal is to make the ewokscore graph analysis more intelligent (discussion: https://confluence.esrf.fr/display/AAWWK/Graph+analysis) so that it looks at forking and merging of branches, not just looking at ancestor links being required or not.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Workflows that will be affected but the graph analysis change:
|
||
| else: | ||
| submodel21 = submodel21_conditions | ||
| out1_required = None | ||
|
|
||
| nodes = [ | ||
| {"id": "in", "task_type": "method", "task_identifier": qualname(passthrough)}, | ||
|
|
@@ -132,6 +134,7 @@ def workflow21(on_error): | |
| { | ||
| "source": "out1", | ||
| "target": "out", | ||
| "required": out1_required, | ||
| "data_mapping": [{"source_output": "return_value", "target_input": "a"}], | ||
| }, | ||
| { | ||
|
|
||
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.
So the number of retained links is always 1 or 0 ?
What is a "retained link" by the way? 😅
Uh oh!
There was an error while loading. Please reload this page.
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.
There is no such thing as a "retained link". Inputs are cached, buffered or retained:
https://ewokscore.readthedocs.io/en/latest/reference/specs.html#summary
Buffered and retained is essentially the same from a link POV: inputs from optional non-cached links.
We buffer until the first execution (which happens when all required links are cached), after which the buffer is purged one-by-one (execute once for each item we purge) and the last one is retained.
Uh oh!
There was an error while loading. Please reload this page.
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.
In the implementation I use two attributes (I added the type annotation in the code)
In the beginning I had only
_buffer_optional_triggerson which we append and pop the inputs from optional non-cached links.This matches the description better but the implementation is horrible due to the appending and popping which also depends on when it happens.
Now we
_buffer_optional_triggersuntil the first execution: keep many to use later (aka buffering)_buffer_optional_triggerson first execution and put the last one in_retained_optional_trigger_retained_optional_trigger: keep only 1 (aka it retains the last one)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.
The number of retained inputs is always 1 or 0.