Add warning color to DAG in UI #245

kenxu95 · 2022-07-22T18:48:41Z

Describe your changes and why you are making these changes

When a warning check fails to pass, we mark the artifact it outputs in a new warning color: orange.

Also, changes the color palette to something with more contrast (cc: @agiron123 as color picker)

Examples below:
I realize these are currently inconsistent as to whether the check operator or the check artifact are highlighted - curious as to what the right colors are in each case. The error check fails the entire workflow, so having the operator be red seems to make sense. I'm thinking I also make the warning operator orange? Or maybe in the future we should squash the check operator and artifact concepts into one. @vsreekanti

Related issue number (if any)

ENG-1265

Checklist before requesting a review

I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
I have performed a self-review of my code.
I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
If this is a new feature, I have added unit tests and integration tests.
I have run the integration tests locally and they are passing.
I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
All features on the UI continue to work correctly.
Added one of the following CI labels:
- run_integration_test: Runs integration tests
- skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

vsreekanti · 2022-07-22T19:11:44Z

I think we can/should make the check artifact and the check operator both red, no? Alternately, we can make the check operator green (it executed successfully) and the check artifact red (it was bad). But my guess is that if a check fails, we should make the operator and artifact both red. @jegonzal @agiron123 any thoughts?

kenxu95 · 2022-07-22T19:33:29Z

Making both of them red is inconsistent with the way we currently do things - if an operator fails, then the artifact is never written and so stays in a pending state, where they can't see the data. Do we want to try and change this larger pattern? I have no idea how hard that would be.

And I don't know if marking only the artifact as red makes sense, because the operator needs to fail on error-severity checks to mark the entire workflow as failed. It would require some custom logic in the UI just for checks in order to pretend like the check operator passed but the artifact failed.

I feel like marking both the warning check operator and artifact as orange is both the easiest and most consistent (warning is equivalent to success for all intents and purposes, except for the fact that a warning will show on the sidebar).

vsreekanti · 2022-07-22T21:11:28Z

So if a check returns False the operator fails, the result will not be written?

kenxu95 · 2022-07-22T21:12:13Z

Only if the check has ERROR severity, yes. It gets written in the WARNING case.

vsreekanti · 2022-07-22T21:19:23Z

Sorry, I think I'm being vague. I was asking if the False value would be persisted, not the artifact that the check is attached to.

kenxu95 · 2022-07-22T21:34:40Z

By attached to you mean the artifact that is the input of the check operator? I think I was interpreting the question correctly then - but I just checked the code and it looks like we do persist the False in either case.

src/ui/common/src/components/workflows/StatusBar.tsx

agiron123 · 2022-07-22T21:36:26Z

integration_tests/sdk/error_handling_test.py

-    except AqueductError as e:
-        assert TIP_OP_EXECUTION in e.message
+    client.publish_flow("hello", artifacts=[processed_artifact])
+    # try:


Is there a reason why you are leaving this code commented out?

agiron123 · 2022-07-22T21:37:14Z

I think we can/should make the check artifact and the check operator both red, no? Alternately, we can make the check operator green (it executed successfully) and the check artifact red (it was bad). But my guess is that if a check fails, we should make the operator and artifact both red. @jegonzal @agiron123 any thoughts?

@vsreekanti I think both of them being red makes more sense since the two nodes are related to one another.

agiron123 · 2022-07-22T21:37:47Z

Left a few comments on the code, but should be good to go after you resolve them @kenxu95

agiron123

Good to go after comments are resolved.

src/ui/common/src/components/workflows/nodes/Node.tsx

kenxu95 · 2022-07-22T22:27:54Z

Ok I just read a bit more of the frontend code and there's quite a bit of entanglement here due to how much we treat checks as a special case through out codebase. For me to implement the best solution I need to know:

What are the principles behind the color coding of failed operators? If we decide to do red-red here, but other non-check operators stay red-gray on failure, is that inconsistency ok? Or should I simply keep it consistent and do red-gray for everything. Or should I try and change everything to red-red.
For warning checks, should I keep it orange-orange, with the principle that orange is really the same as green, except that it comes with a warning message on the sidebar?
When should the user see the value "False" when the select the check artifact? Should they always see it, or only when the check has error severity WARNING. If they always see it, we would once again be inconsistent with how non-check operators work. Currently, when you select a non-check artifact of a failed operator, you get:

vsreekanti · 2022-07-26T00:48:07Z

@kenxu95, let me know if you have any more questions about our conversations on Friday or if you feel like you have enough direction to wrap this up!

kenxu95 · 2022-07-26T00:51:15Z

oh sry meant to update this @vsreekanti I think I have enough direction. Chatted with Wei about this today and decided that a lot of this is better implemented on the backend, since the UI logic was getting super confusing. The backend stuff was done today and I'm currently polishing this up.

got all the executation info built everything except the reads lint context siwtch ready for review uncomment fixed enum annoyance done fixed unit tests made change ready to test and think about some more look ready for review package log lint added warning error message and fixed lint package lock it works, except warnings need to look like warnigns warnings works fix the status bar hold up lint and review

kenxu95 requested review from vsreekanti and agiron123 July 22, 2022 18:48

agiron123 reviewed Jul 22, 2022

View reviewed changes

src/ui/common/src/components/workflows/StatusBar.tsx Outdated Show resolved Hide resolved

agiron123 reviewed Jul 22, 2022

View reviewed changes

agiron123 approved these changes Jul 22, 2022

View reviewed changes

agiron123 reviewed Jul 22, 2022

View reviewed changes

src/ui/common/src/components/workflows/nodes/Node.tsx Outdated Show resolved Hide resolved

kenxu95 force-pushed the eng-1265-add-ability-to-distinguish-between-ui branch from 437acaa to cd83d21 Compare July 26, 2022 05:42

kenxu95 changed the base branch from main to update_checks_in_orchestration July 26, 2022 05:48

kenxu95 force-pushed the eng-1265-add-ability-to-distinguish-between-ui branch from d8240b3 to 46b90b9 Compare July 26, 2022 05:58

kenxu95 mentioned this pull request Jul 26, 2022

Add exec state to get_artifact_result #253

Merged

8 tasks

kenxu95 force-pushed the update_checks_in_orchestration branch from 34fd8f4 to c6e3b4a Compare July 26, 2022 21:50

kenxu95 changed the base branch from update_checks_in_orchestration to exec_state_backfill July 27, 2022 00:01

kenxu95 force-pushed the exec_state_backfill branch from 5da1a85 to 3db4785 Compare July 27, 2022 00:18

kenxu95 force-pushed the eng-1265-add-ability-to-distinguish-between-ui branch from 46b90b9 to 22a0985 Compare July 27, 2022 00:22

kenxu95 force-pushed the exec_state_backfill branch from 3db4785 to 51a2084 Compare July 27, 2022 19:37

agiron123 approved these changes Jul 27, 2022

View reviewed changes

kenxu95 force-pushed the eng-1265-add-ability-to-distinguish-between-ui branch from 22a0985 to 5acc2d7 Compare July 27, 2022 21:15

Base automatically changed from exec_state_backfill to update_checks_in_orchestration July 28, 2022 00:17

Base automatically changed from update_checks_in_orchestration to main July 28, 2022 00:30

kenxu95 added 16 commits July 27, 2022 17:31

built everything except the reads

85bbc29

added migration that adds exec_state column

c985b0d

lint

df4627c

fix

d154bef

got all the executation info

666153e

it works I think

bd63944

ready for final push

dcf6e25

format

b890055

rebase

5ee5395

got the schema version mixed up

6fed936

bad rebase

8e3cac7

added nodeStatus

b3a7efe

lets see what it looks like and rebuild

eef1ac4

fixed it all up!

7f43358

rename

2a0bb2d

kenxu95 force-pushed the eng-1265-add-ability-to-distinguish-between-ui branch from f756018 to 2a0bb2d Compare July 28, 2022 00:32

revert package-lock

84a9e4f

kenxu95 added the run_integration_test Triggers integration tests label Jul 28, 2022

kenxu95 merged commit ca96bd0 into main Jul 28, 2022

kenxu95 deleted the eng-1265-add-ability-to-distinguish-between-ui branch July 28, 2022 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning color to DAG in UI #245

Add warning color to DAG in UI #245

kenxu95 commented Jul 22, 2022 •

edited

Loading

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022 •

edited

Loading

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022

agiron123 Jul 22, 2022

agiron123 commented Jul 22, 2022

agiron123 commented Jul 22, 2022

agiron123 left a comment

kenxu95 commented Jul 22, 2022 •

edited

Loading

vsreekanti commented Jul 26, 2022

kenxu95 commented Jul 26, 2022

Add warning color to DAG in UI #245

Add warning color to DAG in UI #245

Conversation

kenxu95 commented Jul 22, 2022 • edited Loading

Describe your changes and why you are making these changes

Related issue number (if any)

Checklist before requesting a review

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022 • edited Loading

vsreekanti commented Jul 22, 2022

kenxu95 commented Jul 22, 2022

agiron123 Jul 22, 2022

Choose a reason for hiding this comment

agiron123 commented Jul 22, 2022

agiron123 commented Jul 22, 2022

agiron123 left a comment

Choose a reason for hiding this comment

kenxu95 commented Jul 22, 2022 • edited Loading

vsreekanti commented Jul 26, 2022

kenxu95 commented Jul 26, 2022

kenxu95 commented Jul 22, 2022 •

edited

Loading

kenxu95 commented Jul 22, 2022 •

edited

Loading

kenxu95 commented Jul 22, 2022 •

edited

Loading