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

Mapped Tasks not showing cache status correctly. #711

Closed

Conversation

james-union
Copy link
Contributor

@james-union james-union commented Mar 7, 2023

https://github.com/flyteorg/flyteconsole/issues/644

Two issues
(1) It renders cache status incorrectly because it depended on the index of the logs instead of the id. While running, the logs are not in the original order (they are in order of queued/success instead of original order). So need to find correct cache status based on id
(2) the useStyles/hooks should not be used after component is rendered. It broke the app when the execution status is updated. I managed to place it before any render

To reproduce

For this workflow,
https://development.uniondemo.run/console/projects/flytesnacks/domains/development/workflows/map-task.my_cached_map_workflow_1?duration=all

  • Click Launch Workflow button
  • For the parameter, we need to mix some cached numbers and non-cached numbers to reproduce this issue. Right now, 0 ~ 8 are cached. (This will be changed if someone run this more). So for example, if 0 ~ 8 are cached, we can set the param as [9,1,0,12,11,3]
  • Then while running the mapped task, if you go to the Map Execution panel on the right side (by navigating to the execution details and and click the first Map task), you will see some queued & succeeded tasks. Actually the problem is the icons next to the task names. If those are already queued, then it should be the refresh icon which identifies already cached.
  • To reproduce the second issue, you should wait until the Map task is completed while open this right panel. Once it is updated, the right panel will be broken.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

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

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes flyteorg/flyte#644

@james-union james-union self-assigned this Mar 7, 2023
@james-union james-union requested a review from jsonporter March 7, 2023 03:55
@ursucarina
Copy link
Contributor

ursucarina commented Mar 7, 2023

Did you link the correct issue? is there a test wf link?

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #711 (ff611c8) into master (c5cc902) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   66.92%   66.90%   -0.02%     
==========================================
  Files         455      455              
  Lines       11271    11269       -2     
  Branches     2077     2076       -1     
==========================================
- Hits         7543     7540       -3     
- Misses       3728     3729       +1     
Impacted Files Coverage Δ
...ents/common/MapTaskExecutionsList/TaskNameList.tsx 88.88% <60.00%> (-4.22%) ⬇️

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

@james-union
Copy link
Contributor Author

Did you link the correct issue?

Yes. This is for the frontend part.

@@ -68,7 +72,7 @@ export const TaskNameList = ({
color={log.uri ? 'primary' : 'textPrimary'}
onClick={log.uri ? handleClick : undefined}
key={taskLogName}
Copy link
Contributor

@ursucarina ursucarina Mar 7, 2023

Choose a reason for hiding this comment

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

can you add this key to the div that's wrapping this please?

@james-union james-union closed this Mar 7, 2023
@ursucarina ursucarina deleted the james/mapped-task-cache
james/mapped-task-cache branch March 7, 2023 17:51
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.

2 participants