fix(#278): wrong nodeExecutions details are showed in node executions list #277

anrusina · 2022-02-06T04:25:41Z

Issue: Wrong node execution details are shown if scopeId for two items are the same.

It is a partial fix, which solves naming for items with same scopeId, where parent have different scopeId.
Fully solving the issue requires us to do some infrastructure work: https://github.com/flyteorg/flyteconsole/issues/279

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Video walk through through test cases:
Part 1 (~1.5 min): https://share.getcloudapp.com/12u0QnAw (nested, branch, simple dynamic)
Part 2 (30 sec): https://share.getcloudapp.com/X6u8nPQK (branch + dynamic)

This PR:

removes unneeded useNodeExecutionCode (useWorkflow and FetchTasks execution was called too many times)
adds NodeExecutionDetailsContextProvider which wraps NodeExcution's tabs section (which includes Nodes/Graph/ Timeline). This wrapping allows us to perform only one fetchWorkflow request while we are working in the same workflow scope. Additional fetchTaskExecutionList happen only if task wasn't found in the initial workflow, which usually means it was part of a dynamic task and it's info is tied to a specific node execution
Updates Unit tests with minimal changes, to ensure that all previous cases are still working as expected
Updates storybook to properly consume the data
Fixes memory leak issue in NodeExecutionDetailsPanelContent.tsx by adding isMounted pattern.
Fixes issues in Details Panel, when user selects Task tab and then switches to non-task node item: https://share.getcloudapp.com/xQuzN1wO

Tested with next workflows:

Nested:

Branch

https://localhost.demo.nuclyde.io:3000/projects/flytesnacks/domains/development/executions/ynbgv5b4ar?duration=all

Dynamic

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

Behavior should be fully fixed by Infra task: https://github.com/flyteorg/flyteconsole/issues/279

Signed-off-by: Nastya Rusina <nastya@union.ai>

anrusina · 2022-02-06T04:28:03Z

src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx

                    </ExecutionContext.Provider>
                </NodeExecutionsRequestConfigContext.Provider>
            </QueryClientProvider>
        );

+    describe('when rendering the DetailsPanel', () => {


This test is moved as is with addition of fetchWorkflow mock implementation from the bottom of the file, as I wasn't able to fully clear jest.spyOn() effect, which was messing up with this test return.
added:

fetchWorkflow.mockImplementation(() => Promise.resolve(fixture.workflows.top) );

codecov · 2022-02-06T04:30:40Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1e5432f). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #277   +/-   ##
=========================================
  Coverage          ?   65.77%           
=========================================
  Files             ?      380           
  Lines             ?     8438           
  Branches          ?     1538           
=========================================
  Hits              ?     5550           
  Misses            ?     2888           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e5432f...481aae3. Read the comment docs.

Signed-off-by: Nastya Rusina <nastya@union.ai>

anrusina · 2022-02-07T18:39:41Z

src/components/Executions/ExecutionDetails/ExecutionNodeViews.tsx

-                        <div className={styles.filters}>
-                            <ExecutionFilters {...filterState} />
-                        </div>
+            <NodeExecutionDetailsContextProvider


The only change here: wrapped Tabs content with NodeExecutionDetailsContextProvider. github compare just gone rogue :)

anrusina · 2022-02-07T18:47:20Z

src/components/Executions/types.ts

@@ -4,12 +4,6 @@ import {
    NodeExecutionMetadata,
    WorkflowNodeMetadata
 } from 'models/Execution/types';
-import {


This code and below is removed as not in use anymore

anrusina · 2022-02-07T18:48:09Z

src/models/Graph/convertFlyteGraphToDAG.ts

@@ -75,7 +74,7 @@ export function convertFlyteGraphToDAG(

    // Filter out any nodes with no parents (except for the start node)
    const result = values(nodeMap).filter(
-        n => n.id === nodeIds.start || (n.parentIds && n.parentIds.length > 0)
+        n => n.id === startNodeId || (n.parentIds && n.parentIds.length > 0)


nodeIds.start was using same string to compare, replaced with startNodeId for consistency with other code.

jsonporter · 2022-02-10T23:48:03Z

src/components/Executions/ExecutionDetails/NodeExecutionDetailsPanelContent.tsx

    const nodeExecutionQuery = useQuery<NodeExecution, Error>({
        ...makeNodeExecutionQuery(nodeExecutionId),
        // The selected NodeExecution has been fetched at this point, we don't want to
        // issue an additional fetch.
        staleTime: Infinity
    });

+    React.useEffect(() => {
+        let isCurrent = true;


I am not sure I understand this? Won't this always be true?

It would change to false, whenever return() is called (in this case on component unmount)

jsonporter

Left one comment, just needs clarify - otherwise lgtm 👍

## [0.42.1](http://github.com/lyft/flyteconsole/compare/v0.42.0...v0.42.1) (2022-02-11) ### Bug Fixes * **#278:** wrong nodeExecutions details are showed in node executions list ([#277](http://github.com/lyft/flyteconsole/issues/277)) ([15878ab](http://github.com/lyft/flyteconsole/commit/15878abe4e31f79ae3afbf29ab41b66e941e6ee1)), closes [#278](http://github.com/lyft/flyteconsole/issues/278)

flyte-bot · 2022-02-11T00:39:29Z

🎉 This PR is included in version 0.42.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

anrusina added 3 commits February 3, 2022 18:44

fix: wrong nodeExecutions names are shown in the table

572d106

Signed-off-by: Nastya Rusina <nastya@union.ai>

chore: tests updates

018206e

Signed-off-by: Nastya Rusina <nastya@union.ai>

chore: fix dynamic workflows + memory leaks

fbfb223

Signed-off-by: Nastya Rusina <nastya@union.ai>

anrusina commented Feb 6, 2022

View reviewed changes

chore: storybook update

481aae3

Signed-off-by: Nastya Rusina <nastya@union.ai>

anrusina linked an issue Feb 7, 2022 that may be closed by this pull request

Bug: Wrong, duplicated Sub-workflow name in Node view #278

Closed

anrusina marked this pull request as ready for review February 7, 2022 18:34

anrusina requested review from a team, jsonporter and govalt and removed request for a team February 7, 2022 18:35

anrusina commented Feb 7, 2022

View reviewed changes

anrusina changed the title ~~fiw: wrong nodeExecutions details are showed in node executions list~~ fix(#278): wrong nodeExecutions details are showed in node executions list Feb 8, 2022

jsonporter reviewed Feb 10, 2022

View reviewed changes

jsonporter approved these changes Feb 10, 2022

View reviewed changes

anrusina merged commit 15878ab into master Feb 11, 2022

anrusina deleted the narusina/executionDEtails branch February 11, 2022 00:37

flyte-bot added the released label Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#278): wrong nodeExecutions details are showed in node executions list #277

fix(#278): wrong nodeExecutions details are showed in node executions list #277

anrusina commented Feb 6, 2022 •

edited

Loading

anrusina Feb 6, 2022

codecov bot commented Feb 6, 2022 •

edited

Loading

anrusina Feb 7, 2022

anrusina Feb 7, 2022

anrusina Feb 7, 2022

jsonporter Feb 10, 2022

anrusina Feb 11, 2022

jsonporter left a comment

flyte-bot commented Feb 11, 2022

fix(#278): wrong nodeExecutions details are showed in node executions list #277

fix(#278): wrong nodeExecutions details are showed in node executions list #277

Conversation

anrusina commented Feb 6, 2022 • edited Loading

Type

Are all requirements met?

Complete description

Tested with next workflows:

Follow-up issue

anrusina Feb 6, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2022 • edited Loading

Codecov Report

anrusina Feb 7, 2022

Choose a reason for hiding this comment

anrusina Feb 7, 2022

Choose a reason for hiding this comment

anrusina Feb 7, 2022

Choose a reason for hiding this comment

jsonporter Feb 10, 2022

Choose a reason for hiding this comment

anrusina Feb 11, 2022

Choose a reason for hiding this comment

jsonporter left a comment

Choose a reason for hiding this comment

flyte-bot commented Feb 11, 2022

anrusina commented Feb 6, 2022 •

edited

Loading

codecov bot commented Feb 6, 2022 •

edited

Loading