Skip to content
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

fix: execution view children fetch on demand refactor #676

Merged
merged 14 commits into from
Feb 14, 2023

Conversation

olga-union
Copy link
Contributor

@olga-union olga-union commented Jan 30, 2023

Signed-off-by: Olga Nad [email protected]

This fixes a previous refactor on node details UX by placing execution data into a single shared context between List, Graph and Timeline views (to avoid each view re-fetching already fetched data). However, the previous solution would front load the data b recursively fetching each execution's children which we found caused major issues for larger dynamic workflows.

To fix this issue this PR will implement an on-demand fetch for dynamic workflows.

Type

  • Bug Fix
  • Feature
  • Plugin
  • Chore

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

  • Removed childGroups fetch (recursive) from ExecutionNodeViews page
  • Moved childGroups fetch to onToggle in Table and Timeline components - requests trigger when user clicked on expander
  • Moved map tasks fetch to details page to back fill one-off nodeExecutions data, and to ReactFlowGraphComponent to batch them for all known nodeExecutions, so the graph is rendered properly (this will slow down the graph tab, but based on user's feedback, it's ok, if graph tab takes some time to load).
  • Updated Graph nesting logic to support unexpanded subworkflows at level 0

Known issues

  • some unit tests are broken
  • add childGroups fetch in graph on node click
  • fix rerender of graph on childGroups fetch
  • fix compiledWorkflowClosure to check for is_dynamic field (available in node execution's metadata) instead of pulling multiple requests on repeat
  • test filters in Table tab
  • display parent node as nested in graph to indicate it has children
  • child task type stays in Loading state in the Details Sidebar

Follow-up issue

NA

Test workflows

  • normal core.control_flow.subworkflows.parent_wf
  • dynamic core.control_flow.dynamics.wf
  • nested parent core.control_flow.subworkflows.nested_parent_wf
  • map task flyte.workflows.example_map_task.wf
  • small fan-out dynamic workflows.branching_dynamic_example.wf

@jsonporter jsonporter marked this pull request as ready for review February 14, 2023 17:24
james-union
james-union previously approved these changes Feb 14, 2023
Copy link
Contributor

@james-union james-union left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job! LGTM.

ursucarina
ursucarina previously approved these changes Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #676 (70b42e4) into master (ab91dfc) will decrease coverage by 0.45%.
The diff coverage is 56.80%.

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
- Coverage   67.62%   67.18%   -0.45%     
==========================================
  Files         452      453       +1     
  Lines       11194    11210      +16     
  Branches     2059     2065       +6     
==========================================
- Hits         7570     7531      -39     
- Misses       3624     3679      +55     
Impacted Files Coverage Δ
...ls/TaskExecutionNodeRenderer/TaskExecutionNode.tsx 0.00% <0.00%> (ø)
...ponents/Executions/Tables/nodeExecutionColumns.tsx 32.30% <0.00%> (ø)
...nsole/src/components/flytegraph/ReactFlow/types.ts 100.00% <ø> (ø)
packages/console/src/models/Graph/types.ts 100.00% <ø> (ø)
...rc/components/Executions/ExecutionDetails/utils.ts 80.00% <16.66%> (-20.00%) ⬇️
...ackages/console/src/components/Executions/utils.ts 69.02% <19.35%> (-18.78%) ⬇️
...flytegraph/ReactFlow/transformDAGToReactFlowV2.tsx 30.67% <20.00%> (-0.78%) ⬇️
packages/console/src/components/common/utils.ts 39.28% <23.07%> (+5.95%) ⬆️
...s/flytegraph/ReactFlow/ReactFlowGraphComponent.tsx 51.51% <42.85%> (-9.16%) ⬇️
...cutionDetails/NodeExecutionDetailsPanelContent.tsx 81.38% <50.00%> (-9.41%) ⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

olga-union and others added 14 commits February 14, 2023 14:47
Signed-off-by: Olga Nad <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
* chore: fix expander bug

Signed-off-by: Carina Ursu <[email protected]>

* chore: add await everywhere

Signed-off-by: Carina Ursu <[email protected]>

---------

Signed-off-by: Carina Ursu <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Olga Nad <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
This reverts commit 450d144.

Signed-off-by: Jason Porter <[email protected]>
fix: checkForDynamicExecutions
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Carina Ursu <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
Signed-off-by: Carina Ursu <[email protected]>
Signed-off-by: Jason Porter <[email protected]>
@jsonporter jsonporter merged commit 27d4344 into master Feb 14, 2023
@jsonporter jsonporter deleted the olga-union/execution-view-refactor branch February 14, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants