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

UI: Change color for selected node and edges on graph #2458

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Conversation

tito12
Copy link
Contributor

@tito12 tito12 commented Mar 28, 2023

Problem

Currently selected node and edges are not stand out from others too much.
Due to proposition from last comment from this PR #2384

Solution

Selected node and edges highligthed with primary (green) color which more visible than previous.

image

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've included a one-line summary of your change for the CHANGELOG.md (Depending on the change, this may not be necessary).
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

@tito12
Copy link
Contributor Author

tito12 commented Mar 28, 2023

@howardyoo please take a look and approve if it's what you expect :)

@howardyoo
Copy link
Collaborator

@howardyoo please take a look and approve if it's what you expect :)

Tested it and verified! Nice work - the UI now looks more helpful in terms of understanding upstream and downstream dependencies of the job/dataset that was selected. Thank you, @tito12 !

@howardyoo howardyoo self-requested a review March 28, 2023 16:39
Copy link
Collaborator

@howardyoo howardyoo left a comment

Choose a reason for hiding this comment

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

LGTM, Approved.

@harels harels enabled auto-merge (squash) March 28, 2023 17:08
@wslulciuc
Copy link
Member

@tito12, great work! What will the lineage path color be for failed jobs? That is, will the path be red for jobs with a failed state?

@howardyoo
Copy link
Collaborator

@tito12, great work! What will the lineage path color be for failed jobs? That is, will the path be red for jobs with a failed state?

According to this PR, there is no color based on the job's status. However, I think that may be a good idea, to distinguish status based on the color. @tito12 , how difficult would it be to include what Willy described within this PR?

@tito12
Copy link
Contributor Author

tito12 commented Mar 30, 2023

First idea was about to make green and red color due state, but it wasn't possible with current api. That's why I used neutral color with bold style previously and due proposition added green color for selected node and edges. Currently it's not possible to highlight all nodes on graph with state colors to include it here. @wslulciuc @howardyoo

@howardyoo
Copy link
Collaborator

howardyoo commented Mar 30, 2023

First idea was about to make green and red color due state, but it wasn't possible with current api. That's why I used neutral color with bold style previously and due proposition added green color for selected node and edges. Currently it's not possible to highlight all states on graph with state colors to include it here. @wslulciuc @howardyoo

Hi @tito12 ,
in the Marquez UI, when displaying the lineage graph, here are the list of API calls:
image

In these calls, notice the 'runs' api call, which retrieves the 'run' information of the currently selected job (https://marquezproject.github.io/marquez/openapi.html#operation/getRun)
image

The run json object has an attribute called 'STATE' which is COMPLETED at this time.
The list run-states are mentioned here:


which are:

NEW,
RUNNING,
COMPLETED,
ABORTED,
FAILED

So, I guess we might be able to use this state to indicate whether the job in question can be colored, so I believe the API does exist to extract that. I guess we should color NEW, RUNNING, COMPLETED normally, or if we really want to be fancy, maybe have 'running' state color a little differently (like bright green? or fade-blinking in green?), but as for 'ABORTED' and 'FAILED', I guess we should color them as not successful (like yellow for Aborted, and red for failed? - that sounds good to me, but I'll defer to you for the decision).

As for the datasets (not jobs), the data available might be different.
https://marquezproject.github.io/marquez/openapi.html#operation/deleteDataset is the dataset, and
image
and my take is either we use lifecycleState in the version of the dataset (could be CREATE, DROP, TRUNCATE, etc.)
or maybe use createdByRun -> outputVersions.state that's shown below:
image I guess since lifeCycleState can be empty, maybe we should just use the createByRun -> outputVersions.state (would like to gain some opinions if anybody has any).

I guess the dataset keeps the 'createdByRun' to show information of the run (job)'s result - so the idea might be that

  1. if the dataset has 'createdByRun', look at its outputVersions.state.
  2. if the result is COMPLETED, display it normally
  3. however, if the result is ABORTED or FAILED, then show the dataset as 'yellow' and 'red' (or whatever warning / failure color). This is due to the inference that the job run that was responsible to output this dataset has failed, thus the dataset might also have become invalid.

What do you think?
If re-using the above mentioned outputs (since it looks like those apis are getting called every time the lineage graph is drawn) to highlight colors differently could be possible, I guess it might meet what @wslulciuc wants to see.

@howardyoo
Copy link
Collaborator

I've investigated a little bit into this, but for now, it seems like the way the data is coming from the API, and how the lineage graph does NOT have anything related to 'status' suggests that it is not possible for it to render any status (like error conditions) to it. That should be considered as another improvement for the future.

@harels harels merged commit 4bf01b5 into main Apr 6, 2023
@harels harels deleted the graph-selected branch April 6, 2023 21:02
Xavier-Cliquennois pushed a commit to Xavier-Cliquennois/marquez that referenced this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants