feat(frontend): Complete Sub-DAG implementation in KFPv2. #8326
Conversation
frontend/src/lib/v2/DynamicFlow.ts
Outdated
| elems: PipelineFlowElement[], | ||
| executions: Execution[], | ||
| events: Event[], | ||
| artifacts: Artifact[], | ||
| ) { | ||
| ): PipelineFlowElement[] { | ||
| // IMPORTANT: PipelineFlowElement update is in-place. |
There was a problem hiding this comment.
In the current design, the element update is no longer in-place. Can you either remove the comment or change the comment? Thank you.
There was a problem hiding this comment.
Good point! Removed the comment.
| target: getTaskNodeKey(inputTaskKey), | ||
| arrowHeadType: ArrowHeadType.ArrowClosed, | ||
| }; | ||
| flowGraph.push(edge); | ||
| } |
There was a problem hiding this comment.
Can these two conditions also handle the input artifact -> subDAG and subDAG -> output artifact ?
There was a problem hiding this comment.
subDAG -> output artifact is able to be handled already before this PR. input artifact -> subDAG support is added in this PR.
| } | ||
| return {}; | ||
| } | ||
|
|
||
| function getTaskNameToExecution(executions: Execution[]): Map<string, Execution> { | ||
| const map = new Map<string, Execution>(); | ||
| function getTaskNameToExecution(executions: Execution[]): Map<string, Execution[]> { |
There was a problem hiding this comment.
(Correct me if I am wrong.) I think the function name getTaskNameToExecution is a little confusing, especially we are trying do some set works. Would it be better if we change the name to setTaskNameToExecution? If you think the updated function name is better, can you also change the names for other functions?
There was a problem hiding this comment.
setXXX() is a naming pattern for changing existing object, which means executions: Execution[] parameter in this case. However, we are returning Map<string, Execution[]> which is the function caller tries to get, so I use getXXX() pattern here.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlyaoyuli 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 |
|
/test kubeflow-pipeline-e2e-test |
|
/retest |
|
/lgtm |
Description of your changes:
mlmdIdto the node, because there are multiple Executions/Artifacts with the same name, we need MLMD ID to identify runtime status within a MLMD object uniquely.parent_dag_idwith thelast element in breadcrumb layer's execution ID.for-loophas two iterations:for-loop.0andfor-loop.1. Both iterations are rendered as Sub DAG nodes. And breadcrumb layer will show the iteration name if you open them.flowElementsin place,updateFlowElementsState()returns an updatedflowElementswith runtime status.Video:
Screen.Recording.2022-10-01.at.11.57.43.PM.mov
Checklist: