-
Notifications
You must be signed in to change notification settings - Fork 1.3k
plots: introduce support for images #6431
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
Conversation
deabe35 to
f51aeb0
Compare
e150b4c to
bb5d920
Compare
|
Hi @pared, what's the status on this PR? Is there anything we can do to move it along while you are out? |
44afa56 to
a04558c
Compare
| ) | ||
| else: | ||
| logger.warning("'%s' was not found at: '%s'.", path_info, rev) | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing - even if we want to have a warning that some file does not exist, we need to try to open that file later, so that we get FileNotFound in results, for API consistiency.
|
@dberenbaum I believe it should be fine as-is for now. There could be some polishing in the future, but it should be fine for now. |
5ba0f96 to
4ffef00
Compare
|
Sample image plots rendering: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is isolated, so let's merge and address things in follow-ups (docs too).
| metrics, plots = out.repo.live.show(str(out.path_info)) | ||
|
|
||
| html_path = out.path_info.with_suffix(".html") | ||
| html_path = out.path_info.fspath + "_dvc_plots" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pared What is the reason for this change?
Should we update on DVCLive that {path}.html is no longer the default html output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! To support images, the image files must be stored alongside the html, so the output is now a directory instead of a single file.
I would probably opt to do something like dvcvlive/dvc_plots rather than append them with underscores. Any other ideas are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong preference but, if we go with dvcvlive/dvc_plots, the .html output will now be handled like the .tsv plots (i.e. based onlive's cache option).
Previously, the .html was not tracked by DVC and it was up to the user whether to track it with Git/DVC or just ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's probably why @pared did it this way. Maybe just dvclive_plots then?
Another option would be to reorganize dvclive outputs so that the tsv files are under a subdirectory like dvclive/logs or dvclive/tsv and the html is another subdirectory like dvclive/plots or dvclive/html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like dvclive/tsv & dvclive/html.
Honestly, the word plots for referring to the HTML sounds kind of confusing to me because the .tsv files are also dvc plots in a sense.
Moving the outputs to subdirectories would require some changes in DVC core but it feels like a good direction. I might consider also moving the summary .json to dvclive/summary.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can continue the discussion in treeverse/dvclive#148 as this is a comment in a closed P.R.
| def __init__(self): | ||
| super().__init__( | ||
| "Plot data extraction failed. Please see " | ||
| "https://man.dvc.org/plot for supported data formats." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be /plots
| rel_img_path = relpath(img_path, page_dir_path) | ||
| with open(img_path, "wb") as fd: | ||
| fd.write(image_data) | ||
| return """ | ||
| <div> | ||
| <p>{title}</p> | ||
| <img src="{src}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can probably inject HTML code here:
>>> os.path.relpath('#"> <script...')
'#"> <script...'
For #6145
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. plots: document images supportΒ dvc.org#2839
Thank you for the contribution - we'll try to review it as soon as possible. π
EDIT:
Key takeaways:
repo.plotsno longer returns vega json - now its data collected from particular recvisions - @efiop I think we might need to bump minor version with this one.repo- it still requires repo for some logic related to templates and config, but we might be able to get rid of that too--show-vegaremains - backward compatibility, in case of image target we just won't get any resultWhat we need to document:
plots modifywill not have an effect on image plots (not restricting it as we support dirs, which can have mixed content)Further development: