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

plots: Pass templates_dir to match_renderers. #7820

Merged

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented May 27, 2022

We were forgetting to pass the default template dir.

Fixes #7817.

@daavoo daavoo added bugfix fixes bug A: plots Related to the plots labels May 27, 2022
@daavoo daavoo requested review from dberenbaum and pared May 27, 2022 08:41
@daavoo daavoo requested a review from a team as a code owner May 27, 2022 08:41
@daavoo daavoo self-assigned this May 27, 2022
@dberenbaum
Copy link
Contributor

@daavoo I'm still getting the same error from #7817. Am I missing something?

$ dvc plots show -v -t custom
2022-05-27 11:06:40,087 ERROR: unexpected error - Template 'custom' not found.
------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dave/Code/dvc/dvc/cli/__init__.py", line 185, in main
    ret = cmd.do_run()
  File "/Users/dave/Code/dvc/dvc/cli/command.py", line 22, in do_run
    return self.run()
  File "/Users/dave/Code/dvc/dvc/commands/plots.py", line 77, in run
    renderers = match_renderers(
  File "/Users/dave/Code/dvc/dvc/render/match.py", line 57, in match_renderers
    renderer_class(datapoints, filename, **plot_properties)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dvc_render/vega.py", line 37, in __init__
    self.template = get_template(
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dvc_render/vega_templates.py", line 562, in get_template
    raise TemplateNotFoundError(template)
dvc_render.vega_templates.TemplateNotFoundError: Template 'custom' not found.
------------------------------------------------------------
2022-05-27 11:06:41,078 DEBUG: Version info for developers:
DVC version: 2.10.3.dev94+g069ef835c
---------------------------------
Platform: Python 3.10.2 on macOS-12.3.1-arm64-arm-64bit
Supports:
        azure (adlfs = 2022.4.0, knack = 0.9.0, azure-identity = 1.7.1),
        gdrive (pydrive2 = 1.10.1),
        gs (gcsfs = 2022.5.0),
        hdfs (fsspec = 2022.5.0, pyarrow = 7.0.0),
        webhdfs (fsspec = 2022.5.0),
        http (aiohttp = 3.8.1, aiohttp-retry = 2.4.5),
        https (aiohttp = 3.8.1, aiohttp-retry = 2.4.5),
        s3 (s3fs = 2022.5.0, boto3 = 1.21.21),
        ssh (sshfs = 2022.3.1),
        oss (ossfs = 2021.8.0),
        webdav (webdav4 = 0.9.4),
        webdavs (webdav4 = 0.9.4)
Cache types: <https://error.dvc.org/no-dvc-cache>
Caches: local
Remotes: https
Workspace directory: apfs on /dev/disk3s1s1
Repo: dvc, git

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!
2022-05-27 11:06:41,079 DEBUG: Analytics is disabled.

@daavoo
Copy link
Contributor Author

daavoo commented May 27, 2022

@daavoo I'm still getting the same error from #7817. Am I missing something?

I missed an extra s in templates_dir in dvc_render 🀦 . Will adjust here to use template_dir

@daavoo daavoo force-pushed the 7817-plots-custom-templates-not-being-read-from-dvcplots branch from 069ef83 to 1199d00 Compare May 27, 2022 16:03
@daavoo daavoo requested a review from dberenbaum May 27, 2022 16:04
@daavoo daavoo force-pushed the 7817-plots-custom-templates-not-being-read-from-dvcplots branch from 1199d00 to 0373b5c Compare May 27, 2022 16:04
@@ -240,6 +240,36 @@ def test_plots_path_is_quoted_and_resolved_properly(
assert expected_url in out


def test_should_pass_template_dir(tmp_dir, dvc, mocker, capsys):
Copy link
Member

@skshetry skshetry May 27, 2022

Choose a reason for hiding this comment

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

Both of these tests are not useful. I'll suggest testing actual behaviour instead if you can or get rid of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of these tests are not useful.

@skshetry Could you elaborate on why are not useful?

I'll suggest testing actual behaviour instead if you can or get rid of them.

Behavior is tested in https://github.com/iterative/dvc-render/blob/main/tests/test_templates.py#L39

Comment on lines 77 to +80
renderers = match_renderers(
plots_data=plots_data, out=renderers_out
plots_data=plots_data,
out=renderers_out,
templates_dir=self.repo.plots.templates_dir,
Copy link
Member

Choose a reason for hiding this comment

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

Also, it'd be better to push more of this logic inside the API so that you can test them easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my lack of understanding, could you elaborate on what does "the API" refers to?

We were forgetting to pass default template dir.

Fixes #7817.
@daavoo daavoo force-pushed the 7817-plots-custom-templates-not-being-read-from-dvcplots branch from 0373b5c to 02ba8ce Compare May 30, 2022 14:40
@daavoo
Copy link
Contributor Author

daavoo commented May 30, 2022

@dberenbaum This works for me, could you try with latest version?

@daavoo daavoo merged commit 3d79177 into main May 31, 2022
@daavoo daavoo deleted the 7817-plots-custom-templates-not-being-read-from-dvcplots branch May 31, 2022 14:18
@daavoo
Copy link
Contributor Author

daavoo commented May 31, 2022

Will address test comments in separate P.R. after clarification

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 bugfix fixes bug
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

plots: custom templates not being read from .dvc/plots
4 participants