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

feat: mark memoized node as cached #13883

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MenD32
Copy link

@MenD32 MenD32 commented Nov 8, 2024

Fixes #13848

Motivation

when using memoization, its much easier to debug workflows when you can see that nodes have been cached. currently in large workflows you'd have to go over all the nodes one by one to understand if they are cached, or you could go to the timeline tab.
both of those options offer a bad user experience, that becomes worse as the workflows get larger.

Modifications

changed the icon of successful cached nodes to a database icon (from font awesome) to mark them as cached.

Verification

tested in the UI that it works using the examples/memoized-simple.yaml example

screenshots:

first workflow (no cache hit)
Screenshot 2024-11-08 at 14 53 33

second workflow (cache hit)
Screenshot 2024-11-08 at 14 53 50

@MenD32 MenD32 force-pushed the feat/mark-memoized-node-as-cached branch from 0560b4e to fd44dd5 Compare November 8, 2024 16:19
@MenD32 MenD32 marked this pull request as ready for review November 8, 2024 16:25
@elliotgunton
Copy link

I like the choice of icon - for the abstract concept of "cached" it seems to align with the rest of the world https://www.google.com/search?q=icon+to+represent+cache

But replacing the check mark within the node itself might be confusing for existing and new users alike - it looks like it's a step related to database access, and two workflows running the same thing will look quite different. I would suggest an additive approach e.g. an overlaid database symbol - quick sketch from a google slide

image

Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

I agree with #13883 (comment), that the complete change of icon isn't good UX, and would prefer something similar to @elliotgunton's suggestion.

MenD32 added 2 commits December 8, 2024 10:07
…/argo-workflows into feat/mark-memoized-node-as-cached

Signed-off-by: MenD32 <[email protected]>
@MenD32
Copy link
Author

MenD32 commented Dec 8, 2024

I've added the requested change, I'm not much of a UI UX designer so if you have any changes you'd like to have to the colors and what not, please share

@MenD32
Copy link
Author

MenD32 commented Dec 8, 2024

Screenshot 2024-11-26 at 18 21 49

this is the new icon (when cached)

Signed-off-by: MenD32 <[email protected]>
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.

Mark memoized node as cached
3 participants