plots: support opening plots file directly in the browser by skshetry · Pull Request #5584 · treeverse/dvc

skshetry · 2021-03-09T16:20:21Z

$ dvc plots show --open

I don't know if this is that useful, but I think this does save some time (I was feeling the need when I was going through example-get-started).

Alternative:

No way to be compatible across different platforms. For me on Linux, it's following:

$ dvc plots show | xargs xdg-open

Working on macOS and on Windows (cc @jorgeorpinel?) confirmation would be great.

❗ 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.
document plots diff/show --open flag dvc.org#2293

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

efiop · 2021-03-09T16:46:14Z

@skshetry There doesn't seem to be any comments/requests from users thus far, worried that we are adding dead code that no one is going to use. Though it is possible that people don't know they want it.

shcheklein · 2021-03-09T19:00:37Z

My workflow for this is to use Cmd (~Ctrl on Mac) + click the link DVC prints - it opens it automatically in the browser. Not saying that this option is not needed, just sharing my thoughts on why this feature didn't cross my mind I guess. I wonder how many users are familiar with hot keys in their terminals.

pared

I would add --open to tests/unit/command/test_plot.py. Also, we have live command that produces HTML. Though the latter can be handled in other issue, as it seems we could share some options between plots and live.

dberenbaum · 2021-03-09T21:12:39Z

I think it's a nice idea, although I guess it's a bit of an anomaly since no other commands open up anything outside the terminal. I would use it! Anything to avoid using my trackpad/mouse. It's also possible that some users are a bit confused by seeing a file:// url and don't realize it should be opened in the web browser. But I have no strong feelings on this, so feel free to drop it if it feels unnecessary.

skshetry · 2021-03-10T05:11:20Z

-
-            logger.info(f"file://{path}")
-
+            url = f"file://{path}"


This might need to be quoted, will do in a separate PR.

pared · 2021-03-11T10:29:43Z

+def test_plots_diff_open(tmp_dir, dvc, mocker, caplog):
+    mocked_open = mocker.patch("webbrowser.open", return_value=True)
+    cli_args = parse_args(["plots", "diff", "--targets", "datafile", "--open"])
+    cmd = cli_args.func(cli_args)
+    mocker.patch(
+        "dvc.repo.plots.diff.diff", return_value={"datafile": "filledtemplate"}
+    )
+
+    assert cmd.run() == 0
+
+    expected_url = f"file://{tmp_dir / 'plots.html'}"
+    assert expected_url in caplog.text
+
+    mocked_open.assert_called_once_with(expected_url)
+
+
+def test_plots_diff_open_failed(tmp_dir, dvc, mocker, caplog):
+    mocked_open = mocker.patch("webbrowser.open", return_value=False)
+    cli_args = parse_args(["plots", "diff", "--targets", "datafile", "--open"])
+    cmd = cli_args.func(cli_args)
+    mocker.patch(
+        "dvc.repo.plots.diff.diff", return_value={"datafile": "filledtemplate"}
+    )
+
+    assert cmd.run() == 1
+
+    expected_url = f"file://{tmp_dir / 'plots.html'}"
+    mocked_open.assert_called_once_with(expected_url)
+
+    error_message = "Failed to open. Please try opening it manually."
+    assert caplog.record_tuples == [
+        ("dvc.command.plots", logging.INFO, expected_url),
+        ("dvc.command.plots", logging.ERROR, error_message),
+    ]


I was thinking about adding --open option to test_plots_diff, this exceeds expectations!

efiop · 2021-03-12T02:24:06Z

@skshetry Btw, looks like even though the checkbox is ticket, the doc is missing?

skshetry · 2021-03-12T03:56:20Z

@skshetry Btw, looks like even though the checkbox is ticket, the doc is missing?

I was not sure if this was going to merged. I have created the docs, thanks.

jorgeorpinel · 2021-03-17T18:47:06Z

Working on macOS and on Windows (cc @jorgeorpinel?) confirmation would be great.

I'm on Windows but on WSL (non-GUI Ubuntu) and it fails there unfort:

$ dvc plots show --open train.json
file:///home/jop/DVC-repos/test/plots.html
Start : This command cannot be run due to the error: The system cannot find the file specified.
At line:1 char:1
+ Start "file:///home/.../plots.html"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [Start-Process], InvalidOperationException
    + FullyQualifiedErrorId : InvalidOperationException,Microsoft.PowerShell.Commands.StartProcessCommand

jorgeorpinel · 2021-03-17T18:56:44Z

On Windows / Cmder (Git Bash) it works for me! But only from a DVC project dir.

Minor: for some reason if you run it outside a DVC project it get's stuck (while on Linux that seems to work regardless of where you are).

jorgeorpinel · 2021-03-17T19:01:15Z

Same on regular Command Prompt (cmd):

plots show (in general) gets stuck if run outside a DVC project dir.
plots show --open works from within a DVC project 🙂

skshetry · 2021-03-22T16:43:12Z

@jorgeorpinel, opened #5670 to address the WSL related issue.

To open into a browser in the host computer, you have to specify BROWSER envvar, something like follows:

export BROWSER=/mnt/c/Program Files (x86)/Google/Chrome/Application/chrome.exe

and then you can do --open and it'll open in the specified browser. For me, it's terribly slow.

skshetry · 2021-03-22T16:44:17Z

@jorgeorpinel, for other issues, please open an issue as it needs more information on what's happening. Thanks. 🙂

plots: support opening plots file directly in the browser

bb797d6

skshetry added the feature is a feature label Mar 9, 2021

skshetry requested review from efiop and pared March 9, 2021 16:20

skshetry self-assigned this Mar 9, 2021

pared reviewed Mar 9, 2021

View reviewed changes

add tests

749f52f

skshetry commented Mar 10, 2021

View reviewed changes

fix path on windows tests

19112e3

efiop requested a review from pared March 10, 2021 09:30

pared approved these changes Mar 11, 2021

View reviewed changes

efiop merged commit d245008 into treeverse:master Mar 12, 2021

skshetry deleted the open-plots-directly branch March 12, 2021 02:33

skshetry mentioned this pull request Mar 12, 2021

document plots diff/show --open flag treeverse/dvc.org#2293

Merged

skshetry mentioned this pull request Mar 22, 2021

plots: quote and resolve paths properly #5670

Merged

2 tasks

dberenbaum mentioned this pull request Apr 26, 2022

live: Handle auto_open via env. treeverse/dvclive#243

Merged

Conversation

skshetry commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Alternative:

Uh oh!

efiop commented Mar 9, 2021

Uh oh!

shcheklein commented Mar 9, 2021

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Mar 9, 2021

Uh oh!

skshetry Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

pared Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

efiop commented Mar 12, 2021

Uh oh!

skshetry commented Mar 12, 2021

Uh oh!

jorgeorpinel commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeorpinel commented Mar 17, 2021

Uh oh!

jorgeorpinel commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry commented Mar 22, 2021

Uh oh!

skshetry commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

skshetry commented Mar 9, 2021 •

edited

Loading

jorgeorpinel commented Mar 17, 2021 •

edited

Loading

jorgeorpinel commented Mar 17, 2021 •

edited

Loading