-
Notifications
You must be signed in to change notification settings - Fork 790
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
Store information about the DAG being executed in an artifact #822
Conversation
This will allow the UI and the default card to easily get the DAG even if the code is not packaged. TODO: Provide methods in the client
metaflow/task.py
Outdated
@@ -92,6 +92,18 @@ def property_setter( | |||
setattr(cls, var, property(fget=lambda _, val=val: val, fset=set_cls_var)) | |||
vars.append(var) | |||
|
|||
# We also passdown _graph_artifact through the entire graph | |||
setattr( |
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.
we have the setattr(...property(fget=lambda ...
stanza repeated three times in this function. Maybe you can lift it to a nested function or something, since it is cryptic enough without repetition
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 can try but they are all different setattr. Let me see if I can make it a little less cryptic.
|
||
graph_artifact = { | ||
"file": os.path.basename(os.path.abspath(sys.argv[0])), | ||
"parameters": parameters_info, |
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 am just curious what is the purpose of these parameters here? Are these the function arguments for the join
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.
This is to make explicit what are parameters and what are class-level "constants". Currently, we have no good way of telling these apart and the UI will, for example, display everything that is set at a class level (whether or not it is a parameter) as a Parameter. This will help it address this. It's mostly informational.
Have you had success trying this DAG parsing out with Flows written with R yet? One of the major shortcomings of the current implementation on the UI backend side is that we are unable to parse R flows due to the code package structure. If this feature solves the language specific issues, it would be wonderful :) Otherwise, or perhaps in addition, in order to achieve the result that any flow with a remote datastore has a displayable DAG (for the UI that is), it would be simpler for the time being to focus on pushing the code package to the datastore for all types of execution, and tackling the issues with R flows separately. |
I believe this should solve the R flows as well since R is a wrapper around the Python flow. I could be wrong though (haven't tried) but conceptually, it should work. |
metaflow/graph.py
Outdated
def node_to_dict(name, node): | ||
return { | ||
"name": name, | ||
"type": node.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.
let's map types to something more sensible since the internal names are not super clear, e.g.
TYPE_MAP = {
'foreach': 'split-foreach',
'split': 'split-static',
'linear': 'linear',
'end': 'end',
'join': 'join'
}
This will allow the UI and the default card to easily get the DAG
even if the code is not packaged.
TODO: Provide methods in the client