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

Show notification on a global plots failure #3222

Closed
shcheklein opened this issue Feb 4, 2023 · 9 comments
Closed

Show notification on a global plots failure #3222

shcheklein opened this issue Feb 4, 2023 · 9 comments
Assignees
Labels
A: plots Area: plots webview, side panel and everything related bug Something isn't working priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

shcheklein commented Feb 4, 2023

If dvc.yaml is broken, or something else is happening with plots diff command, we should notify users. Not suggest to add an experiment even though an experiments is already selected, + it goes into a loop (expected?) of trying to get it:

Screen.Recording.2023-02-04.at.3.24.25.PM.mov
@shcheklein shcheklein added bug Something isn't working priority-p1 Regular product backlog A: plots Area: plots webview, side panel and everything related labels Feb 4, 2023
@shcheklein
Copy link
Member Author

@dberenbaum even after fixing the dvc.yaml command is breaking since HEAD is broken. We should fix dvc plots diff to be resilient to a failure in revision, wdyt?

@dberenbaum
Copy link
Contributor

@dberenbaum even after fixing the dvc.yaml command is breaking since HEAD is broken.

@shcheklein What do you meant here? What is breaking after you fix dvc.yaml?

Here's what I see:

Screen.Recording.2023-02-06.at.9.22.05.AM.mov

@shcheklein
Copy link
Member Author

Okay, I can't reproduce this anymore. We can't get back to this if I hit it again.

@shcheklein
Copy link
Member Author

I see it again:

Screen.Recording.2023-02-10.at.10.50.47.AM.mov

This time it's stuck on "Loading plots ..."

@shcheklein shcheklein reopened this Feb 10, 2023
@dberenbaum
Copy link
Contributor

@shcheklein I guess those failed revisions were before the dvclive 2.0 changes and have a different field name? So I guess we (VS Code) should show that error?

Also, do you know if the command returns the diff for the non-failed revisions? We should be able to show a plot for some revisions.

@shcheklein
Copy link
Member Author

Also, do you know if the command returns the diff for the non-failed revisions? We should be able to show a plot for some revisions.

No, it breaks in the middle. Generates a partial result, no HTML.

I guess those failed revisions were before the dvclive 2.0 changes and have a different field name? So I guess we (VS Code) should show that error?

Yes, most likely. Yes, we should throw an error at least. At best we could show non-broken revision (but that depends on DVC).

@dberenbaum
Copy link
Contributor

Opened iterative/dvc#9025.

Possible steps to remediate:

  1. Show the error in VS Code.
  2. Parse the error and take action in VS Code -- for example, drop the broken revision and run plots diff again without it. Does it depend on plots: return error messages for failed plots dvc#7692?
  3. Have DVC return whatever plots it can -- this still requires good errors about what's missing so VS Code can inform the user about what's not showing and why.

@mattseddon
Copy link
Member

mattseddon commented Mar 30, 2023

After #3569 I need to add a global error screen to the plots webview to close this. The screen will be very similar to the content shown for the image with an error below:

image

@mattseddon
Copy link
Member

Was closed by #3627

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 bug Something isn't working priority-p1 Regular product backlog
Projects
None yet
Development

No branches or pull requests

3 participants