forked from yahoo/graphkit
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
4 changed files
with
115 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
""" | ||
This sub-module contains statements that can be used for conditional evaluation of the graph. | ||
""" | ||
|
||
from .base import Control | ||
from .functional import compose | ||
|
||
|
||
class If(Control): | ||
|
||
def __init__(self, condition_needs, condition, **kwargs): | ||
super(If, self).__init__(**kwargs) | ||
self.condition_needs = condition_needs | ||
self.condition = condition | ||
self.order = 1 | ||
|
||
def __call__(self, *args): | ||
self.graph = compose(name=self.name)(*args) | ||
return self | ||
|
||
def _compute_condition(self, named_inputs): | ||
inputs = [named_inputs[d] for d in self.condition_needs] | ||
return self.condition(*inputs) | ||
|
||
def _compute(self, named_inputs): | ||
return self.graph(named_inputs) | ||
|
||
|
||
class ElseIf(If): | ||
|
||
def __init__(self, condition_needs, condition, **kwargs): | ||
super(ElseIf, self).__init__(condition_needs, condition, **kwargs) | ||
self.order = 2 | ||
|
||
|
||
class Else(Control): | ||
|
||
def __init__(self, **kwargs): | ||
super(Else, self).__init__(**kwargs) | ||
self.order = 3 | ||
|
||
def __call__(self, *args): | ||
self.graph = compose(name=self.name)(*args) | ||
return self | ||
|
||
def _compute(self, named_inputs): | ||
return self.graph(named_inputs) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e76a39d
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.
Great to see that you also want conditional nodes in the graph.
I think you should be watching my efforts on this project for the last 2 weeks or so: ae01163
It support an enhanced DAG-solver(yahoo#26) to fix pruning too-much or too-little bugs(yahoo#23, yahoo#24) of this project.
Also operations without all their
need
covered are implicitly excluded from the execution,and this should be very relevant here (yahoo#18).
It has many more enhancements, such as
pytest
, with 77% -->90% coverage;(unfortunately don't have the time to completely fix order for Pythons prior to PY3.5 where dicts are not stable);
Getting back to "conditionals", how do you deal with conditional outputs?
I mean, what if some output is not produced?
e76a39d
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.
(maybe this diagram will fire up your curiosity)
e76a39d
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.
And this is the legend:
e76a39d
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'm not quite sure I understand the picture. What are evicted nodes? What does the step sequence mean?
As for your question, the way it works is a conditional node overrides
__call__
and takes a subgraph. It only executes that subgraph if the condition is true. It looks like there is in fact a bug when a node depends on the output of a conditional branch and that output is not produced.You can see an example here:
https://github.com/syamajala/graphkit/blob/master/test/test_graphkit.py#L233
Some other features I added in my fork of graphkit were the ability to color nodes of the graph and only execute subgraphs of a specific color and type checking. You can specific an optional type for each input and output in order to assure the graph you are constructing is valid.
Also development of the fork has actually moved to here:
https://github.com/slac-lcls/networkfox
e76a39d
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.
steps
DeleteInstruction
must run
operation produces 2 outputs, but one of themoverridden
is also an intermediate given-input, so it must be retained, and not overwritten, thus "pinned".Look ho
This is the overview of the new dag-solver.
Beyond the docstrings, which rhey took some time to rewrite, the pull-requests linked above also explain stuff.
Finally, the test-cases have been significantly retrofitted. You may insert plot commands to view the situation of each new feature.
I think in know what you mean.
When an operation receives only partial inputs, v1.2.4 crashes (that is the "unsatisfied" in yahoo#18).
It has been fixed in my master.
Regarding your new features, do you have them somewhere documented?
e76a39d
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.
Oups, forgotthe Pin diagram.
e76a39d
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 only place they are really documented is in the regression tests:
https://github.com/slac-lcls/networkfox/blob/master/test/test_networkfox.py#L231
https://github.com/slac-lcls/networkfox/blob/master/test/test_networkfox.py#L256
https://github.com/slac-lcls/networkfox/blob/master/test/test_networkfox.py#L269
https://github.com/slac-lcls/networkfox/blob/master/test/test_networkfox.py#L294
e76a39d
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.
Thank you. The colors is an handy extension.
The test-case for the If-Then-Else sparks some questions:
What if i have 2 If's or Else's in a same network?
Is it user-friendly to write such graphs? Or are you planning for an automated tools producing the rules?
Would a generic approach for "optional outputs" cover partially your use-cases?
I mean, what if it was possible to demarcate that after some operation has completed, the graph must re-schedule because some of its outputs are now definitely not produced?
Wouldn't it then be more easy to express the If-Then-Else rules in plain python code, and wrap it up in an operation-with-optional-outputs?
e76a39d
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.
Multiple if's, else's should work.
I found that you need to enforce that they happen in the right order. An If should always proceed an Else and an Else should always succeed an If, doing this you can maintain pointers within each node, as you see here:
https://github.com/slac-lcls/networkfox/blob/master/networkfox/functional.py#L236
I ended up removing ElseIf to simplify since we did not have a need for it, but I think the same principal should apply. I believe this also matters when you try to execute the graph in parallel, but I don't remember all the details.
As for whether it is user friendly or not, it seemed like the most intuitive option to me, aside from having to explicitly state what the subgraph needs and provides in the If/Else statement, it mimics what a normal If/Else statement looks like.
We do not write these graphs by hand. We have a gui tool that generates them.
It seems like an optional output approach might work though? I'd need to think about it some more.
e76a39d
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.
Thank you. It's a pity that your screenshot does not depict an if-then-else component.
Another approach is retrofit the nodes with an optional "conditional gate" in front of their inputs, and express the if-rules with the nodes producing the same output but different conditions. Then you wouldn't need the 3 kjinds of nodes, correct?
You still need an ordering (akin to a "weight), to command the order of evaluation of conditions, but it is more generic.
I would say that In general,