Update frontend handling of graphs #293
Conversation
|
Waiting for @yebrahim before merging. |
| } | ||
|
|
||
| graph.setNode(task.name, { | ||
| bgColor: task.when ? 'cornsilk' : undefined, |
There was a problem hiding this comment.
Nit: This should probably be performed using a CSS class.
There was a problem hiding this comment.
Agreed. This styling is still technically under discussion, so I'll leave it for now, but once it's decided (if we're keeping it), I'll move it to somewhere more appropriate
|
|
||
| // Parent here will be the task that pointed to this DAG template | ||
| if (parent) { | ||
| graph.setEdge(parent, task.name); |
There was a problem hiding this comment.
Will this break if the task names are not globally unique?
There was a problem hiding this comment.
That depends on what you mean by break. It will not crash, but it may not display what you would expect? The nodes have only names as identifiers, so if there are two separate tasks with the same names, they will be represented by a single node within the graph, combining all of their inbound and outbound edges.
There was a problem hiding this comment.
Task names are only unique inside their dag namespace. It's OK to push this now, but in future, the id of a task node should be the full path root-task/level1-task/level2-task/leaf-task which won't clash with root-task/level1-task/level2-2-task/leaf-task
There was a problem hiding this comment.
I can do that in this PR pretty easily but with one caveat: for now, I'll have to assume that tasks can only have siblings as dependencies. Is this a safe assumption to make at this time?
|
/retest |
|
/lgtm |
|
Forgot that we've still not heard from @yebrahim |
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun, rileyjbauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun, rileyjbauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…rt done (#177) * Fixed compilation of dsl.Conditional The compiler no longer produced intermediate steps. * Got rid of _create_new_groups * Changed the sub_group.type check * Update frontend handling of graphs (#293) * Updates the frontend to correctly parse the new format of conditional pipelines * WIP - Assume tasks and templates don't share names * Greatly simplifies graphing of conditional and non-conditional pipelines * Adds/updates StaticParser tests * Give nodes unique names
* Improve the cleanup of Kubeflow deployments. * Use the changes in kubeflow/kubeflow#2344; specify command line arguments for delete_deployment.sh by name not position * Get the zone from the deployment manifest * kubeflow/kubeflow#2344 doesn't depend on iam_patch.yaml; instead it loos for bindings with the specified name. * Handle no clusters in zone. * Fix lint.
* Default cpu/memory limits * Add resource limit tests * Use same variable for requests/limits * Fix limit tests
Updates the
StaticGraphParserand tests to work with the newly simplified format of conditional pipelines.Note: this PR is looking to merge with
avolkov/Fixed-compilation-of-dsl.Conditional-2notmaster.Once this PR is merged, the entire branch will be merged all at once (#177) so that the compiler and frontend stay in sync.
This change is