Skip to content

plots: document html template#2472

Merged
shcheklein merged 20 commits into
masterfrom
2402_document_html_template
Jul 16, 2021
Merged

plots: document html template#2472
shcheklein merged 20 commits into
masterfrom
2402_document_html_template

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented May 13, 2021

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Fixes #2402
Closes #1175

@shcheklein shcheklein temporarily deployed to dvc-org-2402-document-h-8hmmms May 13, 2021 14:04 Inactive
@jorgeorpinel

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Fixing broken links (will commit, please git pull)

Comment thread content/docs/command-reference/config.md Outdated
Comment thread content/docs/command-reference/plots/diff.md Outdated
Comment thread content/docs/command-reference/plots/show.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-8hmmms May 13, 2021 18:15 Inactive
@pared pared changed the title plots: document html tempalte plots: document html template May 14, 2021
@pared

This comment has been minimized.

@iesahin
Copy link
Copy Markdown
Contributor

iesahin commented May 25, 2021

I have edited the guide heavily and had minor edits in ref documents. Guide's title and filename are also changed. Please pull :)

Sidebar doesn't have a link for the document.

@jorgeorpinel

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@iesahin iesahin left a comment

Choose a reason for hiding this comment

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

The web server part seems a bit irrelevant, as it seems possible to link the JS files directly to the template. Browsers should be loading the src of <script> tags from local files if I'm not mistaken. The current guide leads the user to run the web server for their own local plots always.

I would like the web server part removed and template to have <script src="my-vega.js"/> elements instead.


DVC allows to provide a custom HTML template to avoid this requirement.

## Example: Setup local server with Vega libraries
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not possible to skip the web server part and embed the JavaScript files directly in the template with something like <script src="./vega.js"/>? @pared

Copy link
Copy Markdown
Contributor Author

@pared pared May 27, 2021

Choose a reason for hiding this comment

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

This was my initial idea, but I like the server version more due to its robustness:
If we specify server, we are pretty sure the address won't change for the same network. In case of local path, we need to assure two things:

  1. that template path is absolute path (dvc plots produces html's in cwd, so relative paths would require some special logic dedicated to resolving vega libs paths according to cwd)
  2. that all users copying given repo will have the vega libs under the same path

Using local server, I intend to point user to a solution that would be viable for intranet and would work without every user having to put libs in same place, or modify the project each time they change the machine they work on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The final plots document will always need the webserver running in this case, right?

For the intranets, I see your points. That's surely more robust but that configuration can be completely different, e.g., they may already have a central URL to put the files and we don't expect users to copy-paste this template to their workflow.

We just want to show how to supply a template and the user might be looking for a way to render their plots completely differently.

I don't have a strong opinion on this and will trust your judgment. If you see it's better this way, let's keep it. For me, although it has some extra parts, the guide satisfies the purpose of describing the templates. Thank you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that server approach is overkill for example purposes, and tries to direct user into "proper" direction. Maybe lets make an example for local path, and just add note on the bottom of the page why one should consider "static" URL to js?

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey, I managed to modify the guide to use local files.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@iesahin I'm still seeing the web server section so not sure what the conclusion was here.

@pared pared self-assigned this May 28, 2021
@pared pared force-pushed the 2402_document_html_template branch from 88b56ce to 9fdb8e3 Compare May 31, 2021 17:23
@iesahin

This comment has been minimized.

@pared

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

OK I finally took a good look at this and there's lots of info here, which is great. Thanks for all the effort so far @pared and @iesahin ! Here's my high-level review:

TBH if the change is basically to address treeverse/dvc#5679 specifically (see #2402 (comment)), I think the changes are a bit too much. Instead of a whole guide we can probably do with a section or example in the params cmd ref index (https://dvc.org/doc/command-reference/plots). It needs updating anyway since it only mentions Vega templates now.

In summary, I think we should summarize the current guide (quite a bit) and put in the plots index instead, probably both as updates to the Templating sections and as an example.

Also @iesahin I think we have enough info, mind taking this over? 🙏

But @pared def try to keep in mind its better to merge than rebase in docs PRs to avoid force pushing stuff we commit directly from Git (e.g. typos -- we do this often). Thanks!

Co-authored-by: Restyled.io <commits@restyled.io>
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Getting there!

Also, should we wait for a decision on treeverse/dvc#5694 (review)? Cc @pared. Otherwise we should remove the awaiting-dvc-merge label.

Thanks.

Comment on lines +161 to +166
## HTML templates

By default, the plots generated by `dvc plots` require Internet access to load
Vega JavaScript libraries. It's possible to supply a custom HTML file to the
`--html-template` option of `dvc plot show` and `dvc plot diff` when the
Internet access is not available.
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jun 29, 2021

Choose a reason for hiding this comment

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

I don't think we should present this as a solution for offline envs initially. E,g. you can also use this feature even when you're online. Let's move most of that motivation to the example instead please.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So what's a more general motivation for this section that explains the feature? Perhaps something like "The default plots use a plain HTML file which uses cloud Vega libraries." Plus maybe link to the official "Vega libs" location. Is it https://github.com/vega/vega/releases?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another motivation may be to put some extra stuff to the plots, like a banner, title etc. But in general, I think people will understand if something works offline, it can also work online.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jul 6, 2021

Choose a reason for hiding this comment

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

Yes but offline vs online is not the broadest use, right? Templates are about customization in general. Offline js imports just happens to be one thing you can customize.

> `--html-template` provides a custom HTML file to inject plots generated by
> DVC.

You can create an HTML file with `<script>` references to local Vega libraries.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same. That's not the sole use of this feature.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Main motivation is generating plots without Internet access and (maybe) customizing them. References to local Vega libraries captures both. Without Vega, the user cannot produce plots.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jul 6, 2021

Choose a reason for hiding this comment

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

I realize that was the original purpose of the PR but once implemented, it's just one application of HTML template customization, I think.

Comment on lines +281 to +283
### Example for the HTML template

Download the Vega libraries into the directory where you'll produce the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the title be more specific? Something about offline environments or without Internet access.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(And let's start with a quick motivation intro as mentioned earlier 🙂)

Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-7rvxlx June 29, 2021 05:37 Inactive
@pared
Copy link
Copy Markdown
Contributor Author

pared commented Jun 29, 2021

@jorgeorpinel I think the decision is there:
treeverse/dvc#6221 (comment)
I think we can wait till it gets merged.

@shcheklein shcheklein temporarily deployed to dvc-org-2402-document-h-7rvxlx July 6, 2021 10:46 Inactive
@jorgeorpinel jorgeorpinel removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Jul 6, 2021
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Specific suggestions on HTML template example file.

Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
@shcheklein shcheklein temporarily deployed to dvc-org-2402-document-h-7rvxlx July 13, 2021 18:26 Inactive
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Copy edits to HTML templates (will commit)

Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-7rvxlx July 13, 2021 23:44 Inactive
Co-authored-by: Restyled.io <commits@restyled.io>
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-7rvxlx July 13, 2021 23:45 Inactive
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Example copy edits (committing)

Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-7rvxlx July 13, 2021 23:58 Inactive
Co-authored-by: Restyled.io <commits@restyled.io>
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-2402-document-h-7rvxlx July 13, 2021 23:59 Inactive
@jorgeorpinel jorgeorpinel dismissed their stale review July 14, 2021 00:00

Almost there...

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Last questions @iesahin please finalize and lmk for merge (feel free to merge if you can) 👍

Comment on lines +282 to +284
There may be times when you need to produce plots without Internet access, or
want to customize the plots output to put some extra content, like banners or
extra text. DVC allows to replace the HTML file that contains the final plots.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
There may be times when you need to produce plots without Internet access, or
want to customize the plots output to put some extra content, like banners or
extra text. DVC allows to replace the HTML file that contains the final plots.
There may be times when you need to produce plots without Internet access.
DVC allows to replace the HTML file that contains the final plots.

or want to customize the plots output to put some extra content, like banners or extra text

The example isn't about that. Maybe mention these other examples in the Description subsection? Will leave suggestion ⏳

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we can enumerate all use cases of templates in the description? We implement one of the example ideas, the rest can be done in a similar way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't suggesting to enumerate every single possible case. My point was that this "Offline HTML Template" example is not about extra HTML content so that sentence just seems unrelated. Could just be removed IMO

Comment on lines +164 to +165
using the the `--html-template` option. This allows you to customize the
container where DVC will inject plots it generates.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⌛ Extracted from the example:

Suggested change
using the the `--html-template` option. This allows you to customize the
container where DVC will inject plots it generates.
using the the `--html-template` option. This allows you to customize the
container where DVC will inject the plots it generates.
For example, you can add extra content to the web page, like banners or links.

Comment thread content/docs/command-reference/plots/index.md Outdated
@shcheklein shcheklein temporarily deployed to dvc-org-2402-document-h-7rvxlx July 14, 2021 12:17 Inactive
@shcheklein
Copy link
Copy Markdown
Contributor

Merging this to move forward :)

@shcheklein shcheklein merged commit 63e0c00 into master Jul 16, 2021
@shcheklein shcheklein deleted the 2402_document_html_template branch July 23, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ref: Update dvc plots to include information about offline HTML plots cmd: update metrics/plots docs per recent changes

5 participants