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

Image plots show proper message and disable button if image is not available #2277

Closed
shcheklein opened this issue Aug 28, 2022 · 10 comments · Fixed by #3532
Closed

Image plots show proper message and disable button if image is not available #2277

shcheklein opened this issue Aug 28, 2022 · 10 comments · Fixed by #3532
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

Screen Shot 2022-08-27 at 9 00 02 PM

This image doesn't exist at in the experiment, refresh doesn't make much sense here.

Might depend on plots error handing cc @mattseddon @pared

@shcheklein shcheklein added bug Something isn't working A: plots Area: plots webview, side panel and everything related labels Aug 28, 2022
@mattseddon
Copy link
Member

Depends on iterative/dvc#7692 & #1649.

@shcheklein
Copy link
Member Author

@shcheklein revisit this, try to reproduce.

We still need error messages, or a signal that plot doesn't exist in the commit.

@mattseddon
Copy link
Member

The following is a more detailed example of the OP.

dvc.yaml contains a plots entry which is a directory. E.g:

plots:
    - mispredicted

Mispredicted is a directory of images that are produced by a stage:

  inference:
  ...
    outs:
    - mispredicted

Every time an experiment is run the contents of mispredicted can be completely different. Ideally, we would not show errors for missing files because we expect that the contents are different.

@skshetry we need to consider this scenario whilst working through plot errors. Is it possible to do this with the existing DVC code or do we need to add some indicator to the dvc.yaml entry? I think this situation differs from where there are templates missing from a directory between revisions but happy to be corrected. WDYT?

Original dvc.yaml is here.

@mattseddon mattseddon removed the triage label Mar 19, 2023
@mattseddon
Copy link
Member

Can we treat the directory more like a dataset and each image more like a datapoint?

@skshetry
Copy link
Member

dvc plots diff does not know if there's an experiment running, and I don't think it should care. Displaying errors (or not) is contextual, so I think it's up to the extension to interpret.

@dberenbaum
Copy link
Contributor

dberenbaum commented Mar 20, 2023

@skshetry I don't think it's related to whether an experiment is running. One revision contains the file mispredicted/6599-... and the other does not.

@mattseddon Is there an error returned here? I don't see one when testing this. I think we just need to show a different message here than 'No plot to display: refresh` (something like N/A or no image at this revision). I see an output like this:

edit: updated json output to reflect #9146

{
  "data": {
    "dir/img1.png": [
      {
        "type": "image",
        "revisions": [
          "workspace"
        ],
        "url": "/Users/dave/repo/dvc_plots/workspace_dir_img1.png"
      }
    ],
    "dir/img2.png": [
      {
        "type": "image",
        "revisions": [
          "workspace"
        ],
        "url": "/Users/dave/repo/dvc_plots/workspace_dir_img2.png"
      },
      {
        "type": "image",
        "revisions": [
          "main"
        ],
        "url": "/Users/dave/repo/dvc_plots/main_dir_img2.png"
      }
    ]
  }
}

@shcheklein What do you want to show in this scenario?

Some thoughts on what I'd expect:

  • Grouping by directory paths. Seeing all cats, dogs, muffins, etc. together would help organize it.
  • Showing the number of images per revision per directory. At least then I'd easily know whether there are more misclassified dogs or cats in each revision.

@skshetry

This comment was marked as outdated.

@mattseddon
Copy link
Member

mattseddon commented Mar 20, 2023

@mattseddon Is there an error returned here? I don't see one when testing this. I think we just need to show a different message here than 'No plot to display: refresh` (something like N/A or no image at this revision). I see an output like this:

I completely fluffed this. Didn't check the new output. Everything seems to be behaving as it needs to. I will open a PR to close this issue.

Some thoughts on what I'd expect:

  • Grouping by directory paths. Seeing all cats, dogs, muffins, etc. together would help organize it.

I can sort the images before sending them to the webview initially but the user can reorder them which makes future updates more difficult.

  • Showing the number of images per revision per directory. At least then I'd easily know whether there are more misclassified dogs or cats in each revision.

We should have this already in the plots tree. Where should this be shown?

@dberenbaum
Copy link
Contributor

Thanks @mattseddon! Not sure if we want a separate issue for the rest of the discussion for how to organize these images, which is probably not p1.

I can sort the images before sending them to the webview initially but the user can reorder them which makes future updates more difficult.

I meant not just sorting, but making them collapsible by directory.

  • Showing the number of images per revision per directory. At least then I'd easily know whether there are more misclassified dogs or cats in each revision.

We should have this already in the plots tree. Where should this be shown?

🤔 I was thinking if we have collapsible directories, then we could put the number of images per revision next to those. For example, imagine the paths below are directories (cats, dogs, etc.) instead of files (I'm not tied to this UI, it's just the first idea I had):

Screenshot 2023-03-21 at 12 46 32 PM

@shcheklein
Copy link
Member Author

Sorry folks, just catching up a bit. It feels like grouping, etc is a separate improvement that can be done (or not) independently from this ticket?

For this specific ticket - even an empty box (and we don't even need the border) is fine I think.

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

Successfully merging a pull request may close this issue.

4 participants