Support rendering recursive static DAGs #845
Conversation
| } | ||
| // Parent here will be the task that pointed to this DAG template. | ||
| // We do not set an edge if the task has dependencies because in that case it makes sense | ||
| // for incoming edges to only be drawn from its dependencies, not the DAG node itself |
There was a problem hiding this comment.
Can you clarify this comment a little? Why does having or not having dependencies affect having an edge to its parent node?
There was a problem hiding this comment.
This might be a bit wordy, see if you think it makes sense:
// Parent here will be the task that pointed to this DAG template.
// Within a DAG template, tasks can have dependencies on one another, and long chains of
// dependencies can be present within a single DAG. In these cases, we choose not to draw an
// edge from the DAG node itself to these tasks with dependencies because such an edge would
// not be meaningful to the user. For example, consider a DAG A with two tasks, B and C, where
// task C depends on the output of task B. C is a task of A, but it's semantically much more important that
// C depends on B, so to avoid cluttering the graph, we simply omit the edge between A and C:
// A A
// / \ becomes /
// B <-- C B
// /
// C
There was a problem hiding this comment.
Are we sure we won't ever be removing useful information from the graph by doing so?
Separately, the comment is much more clear now, thanks.
There was a problem hiding this comment.
I don't know for certain, but I haven't been able to think of a case where it'd be preferable to see those extra edges, and removing them makes the execution flow of the pipeline a lot clearer (IMO)
rileyjbauer
force-pushed
the
support-recursive-dags
branch
from
46c54d2
to
78de7c8
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
* Adds support for rendering recursive pipelines to the StaticGraphParser * Adds a recursive pipeline to the list of mocks * Fixes recursive graph rendering under new styling * Adds additional comments and surfaces pipeline yaml error to users * Remove unnecessary comment
With this change,
StaticGraphParserno longer breaks when trying to lay out recursive pipelines.This PR also adds a new mock template:
mock-recursive-template.yamlwhich includes multiple instances of recursion.This change is