-
Notifications
You must be signed in to change notification settings - Fork 1.2k
plots: support opening plots file directly in the browser #5584
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
Conversation
@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. |
My workflow for this is to use |
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 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
.
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 |
|
||
logger.info(f"file://{path}") | ||
|
||
url = f"file://{path}" |
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 might need to be quoted, will do in a separate PR.
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), | ||
] |
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 was thinking about adding --open
option to test_plots_diff
, this exceeds expectations!
@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. |
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 |
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). |
Same on regular Command Prompt (
|
@jorgeorpinel, opened #5670 to address the WSL related issue. To open into a browser in the host computer, you have to specify
and then you can do |
@jorgeorpinel, for other issues, please open an issue as it needs more information on what's happening. Thanks. 🙂 |
Closes #4179
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:
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. 🙏