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

Handle DVC errors gracefully in plots #1649

Closed
2 tasks
shcheklein opened this issue May 4, 2022 · 34 comments · Fixed by #3627
Closed
2 tasks

Handle DVC errors gracefully in plots #1649

shcheklein opened this issue May 4, 2022 · 34 comments · Fixed by #3627
Assignees
Labels
A: plots Area: plots webview, side panel and everything related priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

shcheklein commented May 4, 2022

This depends on iterative/dvc#7692

Figma specs

Motivation

  1. Will be missing data for a specific experiment within one of the plots. This will be most apparent in the comparison table. We need a placeholder and some way to refresh the missing experiment's data.
  2. Currently, if an experiment completely fails to load plots data there will be nothing to let the user know the experiment is missing. Add an indicator into the webview's top experiment selection section in order to let the user know that an experiment has something wrong.

TODO

Will be handled in relation to the: DVC errors that may occur during or after running experiment #1636

@shcheklein shcheklein added priority-p1 Regular product backlog 🎨 design Needs design input or is being actively worked on A: plots Area: plots webview, side panel and everything related labels May 4, 2022
@maxagin

This comment was marked as outdated.

@mattseddon

This comment was marked as outdated.

@mattseddon

This comment was marked as outdated.

@mattseddon

This comment was marked as outdated.

@maxagin

This comment was marked as outdated.

@maxagin
Copy link
Contributor

maxagin commented May 17, 2022

@mattseddon I think we may want to use buttons where it is possible. take a look and let me know.

Screen Shot 2022-05-16 at 10 45 37 PM

@mattseddon

This comment was marked as outdated.

@maxagin
Copy link
Contributor

maxagin commented May 17, 2022

Signal user that an experiment has something wrong at the tabs level. We can use the exclamation sign (will be using it for similar cases - errors) colored with the section color.

Screen Shot 2022-05-17 at 12 06 42 AM

Keep the tabs appearance consistent with the expS table and side panel. Wdyt?

Screen Shot 2022-05-17 at 12 06 50 AM

@mattseddon

This comment was marked as outdated.

@maxagin

This comment was marked as outdated.

@maxagin
Copy link
Contributor

maxagin commented May 18, 2022

I would like to finalize this if possible. Please share your comments. @shcheklein and @mattseddon Thanks

@mattseddon
Copy link
Member

@maxagin

This is fine. Please update the designs in the figma. We need to start consolidating all of this information in to one place so we can prioritise what we are going to implement next. We also need to make sure there is no contradictory information anywhere. E.g dots vs strips for the experiment colors.

@mattseddon
Copy link
Member

Implemented the refresh button in #1754. The concept looks strange when there are multiple plots missing:

image

Consider leaving for now but removing the button altogether once the ribbon is in place.

@maxagin
Copy link
Contributor

maxagin commented May 23, 2022

Great ! @mattseddon just a few comments:

The concept looks strange when there are multiple plots missing

The reason is that the area is not defined. With the border it will be fine. See here #1649 (comment)

refresh button

I have noticed that most of the time buttons have significant clear space (padding) on the sides. I propose same, should feel more consistent. See here #1649 (comment)

@mattseddon
Copy link
Member

The reason that I say that it "looks strange" is because we have a refresh button on each plot that does the exact same thing. I do not think that adding a border will make the duplication of information/button look any better:

image

The buttons are from the webview-ui-toolkit. I am using them as provided by MS. Please take a look.

@maxagin
Copy link
Contributor

maxagin commented May 23, 2022

I am using them as provided by MS

I had an impression that we can have it 100% of the parent container, like in the attachment. Sorry if I was mistaken.

Screen Shot 2022-05-22 at 11 33 22 PM

adding a border will make the duplication of information/button look any better

Yes. It won’t do the duplication any better, but it will outline the space with the message. So the user won’t feel like the info is in the air, but it belongs to a specific place. Does it make sense?

we have a refresh button on each plot that does the exact same thing

Do you mean if the user will click one button it will refresh all the missing plots. I thought that one button refreshed info related to the specific plot.
If by clicking one button all plots will be refreshed - then it does not make sense to have a button for each section and the button needs to be located somewhere else ‘general’ refresh.

@mattseddon
Copy link
Member

I am using them as provided by MS

I had an impression that we can have it 100% of the parent container, like in the attachment. Sorry if I was mistaken.

Screen Shot 2022-05-22 at 11 33 22 PM

Generally the only place that you would see a button like that would be in the sidebar panel. Whenever buttons are shown in the toast or any webview-toolkit-ui previews they have much less space between the edge and the text/icons

image

image

image

image

we have a refresh button on each plot that does the exact same thing

Do you mean if the user will click one button it will refresh all the missing plots. I thought that one button refreshed info related to the specific plot. If by clicking one button all plots will be refreshed - then it does not make sense to have a button for each section and the button needs to be located somewhere else ‘general’ refresh.

It will refresh all plots for that revision/experiments. Hence this comment:

Consider leaving for now but removing the button altogether once the ribbon is in place.

@maxagin
Copy link
Contributor

maxagin commented May 23, 2022

Whenever buttons are shown in the toast or any webview-toolkit-ui previews they have much less space between the edge and the text/icons

Yes, but I thought this is also a webview and it uses the full width of the content wrap. No?

Screen Shot 2022-05-23 at 12 32 55 AM

However if the button needs to refresh all plots of a particular exp, it doesn't matter . Thank you @mattseddon

@mattseddon
Copy link
Member

mattseddon commented May 23, 2022

The provided picture is from a walkthrough which probably uses a webview under the hood but it is internal to VS Code and does not use the toolkit.

@maxagin
Copy link
Contributor

maxagin commented Jun 13, 2022

Hey @mattseddon . Please see the Figma specs which I have recently created. Let me know what you think.

@maxagin
Copy link
Contributor

maxagin commented Jun 13, 2022

Well, I have just created a different way of displaying the errors, a part of this is the same.

Screen Shot 2022-06-13 at 7 56 45 PM

So, what we will do here. Do we need the error for the exp tab?

@arcticbear
Copy link

@maxagin
There must be another red color for showing an error on the label which will be better to read. This one is really tough to read, it's painful for the eyes. A suggestion is to use a more light red tone.

Tooltip aligning by the center is confusing. Currently, all the tooltips are aligned by their left side to the parent element, which is consistent with menus and other elements' behavior.

@maxagin
Copy link
Contributor

maxagin commented Jul 7, 2022

There must be another red color for showing an error on the label which will be better to read. @arcticbear

We are trying to follow the native roles as much as possible - VSCode guidelines

Screen Shot 2022-07-07 at 1 59 21 AM

However, for this particular case, we probably won't use red at all (WIP).


Tooltip aligning by the center is confusing.

The context menus are always aligned by the right side, unless it's very close to the right age of the screen, in this case, the context menu will appear on the left side of the mouse. However, in some rear cases, the context menu appears always on the right side.

Regarding the tooltips with content that can be copied or having other actions such as links will be placed at the center of the element. This is consistent with common interaction patterns including GH and Studio

@maxagin
Copy link
Contributor

maxagin commented Jul 12, 2022

The Figma specs were updated. Please let me know if you will have any comments. Thanks

@mattseddon mattseddon removed the 🎨 design Needs design input or is being actively worked on label Aug 2, 2022
@mattseddon mattseddon assigned mattseddon and unassigned maxagin and mattseddon Aug 2, 2022
@mattseddon mattseddon added blocked Issue or pull request blocked due to other dependencies or issues DVC-first Needs to be done first for DVC labels Aug 2, 2022
@shcheklein
Copy link
Member Author

When dvc plots show --json fails like this:

(.venv) ?254 Projects/ensemble-dvc-template % dvc plots show --json
ERROR: unexpected error - Field 'epoch' does not exist in provided data.

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!

We handle it in some weird way in VS Code:

Screen Shot 2022-10-29 at 1 42 08 PM

@shcheklein shcheklein changed the title Plots: handle errors gracefully Handle errors gracefully in plots Nov 5, 2022
@shcheklein shcheklein changed the title Handle errors gracefully in plots Handle DVC errors gracefully in plots Nov 5, 2022
@shcheklein shcheklein self-assigned this Dec 29, 2022
@mattseddon mattseddon self-assigned this Mar 30, 2023
@mattseddon mattseddon removed blocked Issue or pull request blocked due to other dependencies or issues DVC-first Needs to be done first for DVC labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Area: plots webview, side panel and everything related priority-p1 Regular product backlog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants