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

dvc render: Add RENDERERS registry. #6931

Merged
merged 1 commit into from
Nov 15, 2021
Merged

dvc render: Add RENDERERS registry. #6931

merged 1 commit into from
Nov 15, 2021

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Nov 4, 2021

Pre-requisite for iterative/dvc-render#7 -> #4455

These changes should not break any existing behavior.

Refactored existing render / plots.template logic in order to make it more "plugin" friendly so it's easier to add new renderers (i.e. plotly / parallel coordinates plot).

Open questions:

  • Should ImageRenderer implement as_json (meaning a machine-shareable representation of the "plot"). Maybe we should use base64 for that (potentially even internally as src).

  • Should ImageRenderer be, somehow, template-based?. It looks that it could be generalized to follow a similar logic as VegaRenderer where DVC provides "anchor" data (the src in <img>) and the template can customize how to insert that data in the HTML.

  • Is there a reason to have templates in dvc.repo.plots? It looks like there is the only remaining connection to other dvc subsystems that prevents render to be extracted as a separate library.

@daavoo daavoo requested a review from a team as a code owner November 4, 2021 19:12
@daavoo daavoo requested review from skshetry and pared November 4, 2021 19:12
@daavoo daavoo added refactoring Factoring and re-factoring skip-changelog Skips changelog A: plots Related to the plots labels Nov 4, 2021
@daavoo daavoo self-assigned this Nov 5, 2021
@pared
Copy link
Contributor

pared commented Nov 9, 2021

Should ImageRenderer implement as_json (meaning a machine-shareable representation of the "plot"). Maybe we should use base64 for that (potentially even internally as src).

I wouldn't do that until there is demand for that. As far as the discussions go with @rogermparent, more desirable would be to get URL to the images.

Should ImageRenderer be, somehow, template-based?. It looks that it could be generalized to follow a similar logic as VegaRenderer where DVC provides "anchor" data (the src in ) and the template can customize how to insert that data in the HTML.

Lets remember that from HTML point of view, Image and Vega behave very similar - they just provide the <div> containing the visualization. What would such templating do?

Is there a reason to have templates in dvc.repo.plots? It looks like there is the only remaining connection to other dvc subsystems that prevents render to be extracted as a separate library.

Good point, there is no need for that, I moved it as is to VegaRenderer in order to not complicate #6431 which already was humonguous.

@daavoo
Copy link
Contributor Author

daavoo commented Nov 9, 2021

I wouldn't do that until there is demand for that. As far as the discussions go with @rogermparent, more desirable would be to get URL to the images.

I don't really have the context from that discussions. I was just thinking that base64 would technically be the "machine-readable" equivalent to the vega spec that we are returning in --show-vega, because the json actually includes the parsed data.

Regardless, the bigger point was that all renderers should implement a method (as_json?) and we should rename --show-vega to --show-json. It won't matter much if the json representation of the images are URLs or base64, I don't really have a use case requirement for using base64.

Lets remember that from HTML point of view, Image and Vega behave very similar - they just provide the

containing the visualization. What would such templating do?

For vega, we have a DIV in the renderer class:

dvc/dvc/render/vega.py

Lines 102 to 109 in 4175f9f

DIV = """
<div id = "{id}">
<script type = "text/javascript">
var spec = {partial};
vegaEmbed('#{id}', spec);
</script>
</div>
"""

And users can customize the vega spec ({partial}) via templates.

For images, we also have a DIV in the renderer:

dvc/dvc/render/image.py

Lines 13 to 18 in 4175f9f

DIV = """
<div
id="{id}"
style="border: 1px solid;">
{partial}
</div>"""

But in this case users can't customize the {partial}, we hardcode the creation of a children <div>:

dvc/dvc/render/image.py

Lines 35 to 41 in 4175f9f

return """
<div>
<p>{title}</p>
<img src="{src}">
</div>""".format(
title=revision, src=(relpath(img_path, page_dir_path))
)

My perspective here would be that users could use templates to customize this children <div>.

However, I'm not sure if this makes sense or it is just trying too hard to mimic abstractions between renderers for plots and images.

@pared
Copy link
Contributor

pared commented Nov 10, 2021

Regardless, the bigger point was that all renderers should implement a method (as_json?) and we should rename --show-vega to --show-json. It won't matter much if the json representation of the images are URLs or base64, I don't really have a use case requirement for using base64.

The renaming will probably take place, though I would leave --show-vega as a hidden option for backward compatibility, and get rid of it in 3.0. Let's not focus too much on representation, for now, I am just following what seems to be a better fit for dependant projects, we can talk with the VSCode team about that.

My perspective here would be that users could use templates to customize this children

.

Makes sense, the current state of image rendering is very ugly.

assert len(files) == 1
self.filename = files.pop()

def _convert(self, path: "StrPath"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time I see this name I am cringing, if you have some comments on this name feel free to share :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in #6943 names (and logic) could be revisited

@pared
Copy link
Contributor

pared commented Nov 11, 2021

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2021

rebase

✅ Branch has been successfully rebased

@pared
Copy link
Contributor

pared commented Nov 11, 2021

@daavoo can you include the information that is related to iterative/dvc-render#7, #4455, and #6943 in the commit comment? Let's merge it, I am working on #6943 and it will be helpful there.

@pared pared merged commit 4a1985d into master Nov 15, 2021
@pared pared deleted the pluginfy-render branch November 15, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Related to the plots refactoring Factoring and re-factoring skip-changelog Skips changelog
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants