-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: introduce support for images #6431
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
@@ -49,7 +49,6 @@ def _collect_paths( | |||
) | |||
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).
@@ -15,10 +17,10 @@ def create_summary(out): | |||
|
|||
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 iterative/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.plots
no 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-vega
remains - backward compatibility, in case of image target we just won't get any resultWhat we need to document:
plots modify
will not have an effect on image plots (not restricting it as we support dirs, which can have mixed content)Further development: