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

Plots should be more resilient to errors in specific revisions #4333

Open
shcheklein opened this issue Jul 23, 2023 · 18 comments
Open

Plots should be more resilient to errors in specific revisions #4333

shcheklein opened this issue Jul 23, 2023 · 18 comments
Labels
A: plots Area: plots webview, side panel and everything related blocked Issue or pull request blocked due to other dependencies or issues DVC-first Needs to be done first for DVC enhancement New feature or request priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

Screen.Recording.2023-07-23.at.10.36.08.AM.mov
@shcheklein shcheklein added enhancement New feature or request priority-p1 Regular product backlog A: plots Area: plots webview, side panel and everything related labels Jul 23, 2023
@mattseddon
Copy link
Member

I would argue that this is a CLI issue. The extension is only reflecting what is provided by DVC:

image

@shcheklein
Copy link
Member Author

It might be. It's still an issue that we have in the product. Users don't care about the implementation details. We can discuss and create a plan with the DVC team to address this.

@dberenbaum
Copy link
Contributor

dberenbaum commented Jul 24, 2023

Did motor-abac exist at the time?

Edit: okay, I see there's more context in #4332. Should we disable/drop plots for queued and/or failed experiments? The only fix I can see in the CLI would be to still show the other revisions, but I still don't like that UX since I would be wondering where the motor-abac plots are.

@shcheklein
Copy link
Member Author

@dberenbaum good question:

Screenshot 2023-07-24 at 7 28 46 AM

@shcheklein
Copy link
Member Author

Should we disable/drop plots for queued and/or failed experiments?

We have a way to deal with - we show an error in the ribbon

Should we disable/drop plots for queued and/or failed experiments?

We can - it's just also not a good UX - let's say it fails in the middle - plots disappear and there is no atm even a way to tell what went wrong.

@mattseddon
Copy link
Member

mattseddon commented Jul 27, 2023

I think we tried to tackle this before in iterative/dvc#9025

That's not right, I'll open a new issue tomorrow.

@mattseddon
Copy link
Member

Raised iterative/dvc#9776

@mattseddon mattseddon added the DVC-first Needs to be done first for DVC label Jul 31, 2023
@dberenbaum
Copy link
Contributor

What is the end goal of the UX here? It still seems to me that it would be better to disable plots for rows where they don't exist, and this matches the UX in Studio. An error in the ribbon makes sense to me if there is an issue with one plot within a revision, but I don't see the point in showing the revision at all if it has no plots or isn't even a valid git revision.

@shcheklein
Copy link
Member Author

The goal here is to have a "stable" UX for plots. As a user I should be able to pick any experiment to be show in plots and it should stay there all the time until I toggle it back off. It should not be disappearing w/o me as a user understanding why. Users should not care about the internals of the system. If there is an entry in the table it should behave the same as any other entry preferably. Happy to discuss if there are any other ideas here.

@dberenbaum
Copy link
Contributor

I thought this is a case where there are no plots for that experiment yet (it hasn't been run yet, or it failed before plots were generated). Is that the case? Does it make sense to give users an option to view plots for an experiment in that state?

@shcheklein
Copy link
Member Author

so, the full scenario is:

  • I queue multiple exps
  • I select them for plots
  • I run-all them
  • I open plots to follow
  • I want to see them or proper errors

We can indeed disable the plots button until we 100% sure that it can be plotted at all, but it feels like an interface that is hard to explain (unstable, changes its state based on the state of an experiment, etc).

@dberenbaum
Copy link
Contributor

How it looks in Studio now:

Screen.Recording.2023-07-31.at.12.53.35.PM.mov

It doesn't allow you to "pre-select" rows that don't yet have plots, but otherwise I have found it useful and intuitive rather than not knowing what rows have plots until I open the plots view. Is it important enough to "pre-select" all the queued experiments to plot that we need a different UX in VS Code?

@shcheklein
Copy link
Member Author

Even in Studio if I know in advance that it's supposed to be a live experiments with plots and it's just a matter of me waiting for it to receive some data, I would prefer to be able to pick it for plots and don't wait for button to appear.

In case of VS Code in case of those queued experiments we know that plot definitions are there, it would be confusing to not allow that button to my mind. Unless I misunderstand your point.

@mattseddon
Copy link
Member

FWIW you cannot plot queued experiments in the extension before they start:

image

@shcheklein
Copy link
Member Author

yep, but when they start you can immediately plot them, right?

btw, why don't we allow this even before they start? primarily, because we needed a place for an icon?

@mattseddon
Copy link
Member

I thought the issue here was related to an experiment that ended in an error preventing plots diff from returning anything.

The correct behaviour could be to allow selecting the experiment but not passing the name/id to plots diff. Instead, we would show an error in the plots ribbon for that revision. In the case of it being a queued experiment that failed we would give the user the chance to view the log. In order to view the log we would need access to that information... or we try to preload it into the plots data using dvc queue log <exp-name>.

This suggestion might bring us back to iterative/dvc#9442

@mattseddon
Copy link
Member

yep, but when they start you can immediately plot them, right?

Yes, you can follow if you like. We don't track by default.

btw, why don't we allow this even before they start? primarily, because we needed a place for an icon?

  1. Queued experiments don't necessarily have metrics
  2. plots diff using the name errors with ERROR: unknown Git revision 'name'
  3. Assumption is that users running via the queue are not closely monitoring experiments as they run

@dberenbaum
Copy link
Contributor

  1. Queued experiments don't necessarily have metrics

They also frequently have the wrong metrics and plots because they still contain the metrics and plots files from the baseline revision.

We can - it's just also not a good UX - let's say it fails in the middle - plots disappear and there is no atm even a way to tell what went wrong.

Opened iterative/dvc#9787 for experiments that are killed/fail in the middle.

@shcheklein shcheklein added the blocked Issue or pull request blocked due to other dependencies or issues label Aug 22, 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 blocked Issue or pull request blocked due to other dependencies or issues DVC-first Needs to be done first for DVC enhancement New feature or request priority-p1 Regular product backlog
Projects
None yet
Development

No branches or pull requests

3 participants