Skip to content

live: Handle auto_open via env. #243

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.

Sign up for GitHub

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

Merged
daavoo merged 1 commit into from
Apr 26, 2022
Merged

live: Handle auto_open via env. #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dvclive/env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
DVCLIVE_OPEN = "DVCLIVE_OPEN"
DVCLIVE_PATH = "DVCLIVE_PATH"
DVCLIVE_SUMMARY = "DVCLIVE_SUMMARY"
DVCLIVE_HTML = "DVCLIVE_HTML"
Comment thread
dberenbaum marked this conversation as resolved.
Outdated
Expand Down
24 changes: 9 additions & 15 deletions dvclive/live.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from pathlib import Path
from typing import Any, Dict, Optional, Union

from . import env
from .data import DATA_TYPES, PLOTS, Image, Scalar
from .dvc import make_checkpoint
from .error import (
Expand All @@ -15,7 +16,7 @@
InvalidPlotTypeError,
)
from .report import html_report
from .utils import nested_update, open_file_in_browser
from .utils import env2bool, nested_update, open_file_in_browser

logger = logging.getLogger(__name__)

Expand All @@ -28,14 +29,12 @@ def __init__(
path: Optional[str] = None,
resume: bool = False,
report: Optional[str] = "html",
auto_open: bool = False,
):

self._path: Optional[str] = path
self._resume: bool = resume
self._report: str = report
self._checkpoint: bool = False
self._auto_open: bool = auto_open
self.html_path = None

self.init_from_env()
Expand Down Expand Up @@ -77,23 +76,19 @@ def _init_paths(self):
os.makedirs(self.dir, exist_ok=True)

def init_from_env(self) -> None:
from . import env
if os.getenv(env.DVCLIVE_PATH):

if env.DVCLIVE_PATH in os.environ:

if self.dir and self.dir != os.environ[env.DVCLIVE_PATH]:
if self.dir and self.dir != os.getenv(env.DVCLIVE_PATH):
raise ConfigMismatchError(self)

env_config = {
"_path": os.environ.get(env.DVCLIVE_PATH),
"_checkpoint": bool(
int(os.environ.get(env.DVC_CHECKPOINT, "0"))
),
"_resume": bool(int(os.environ.get(env.DVCLIVE_RESUME, "0"))),
"_path": os.getenv(env.DVCLIVE_PATH),
"_checkpoint": env2bool(env.DVC_CHECKPOINT),
"_resume": env2bool(env.DVCLIVE_RESUME),
}

# Keeping backward compatibility with `live` section
if not bool(int(os.environ.get(env.DVCLIVE_HTML, "0"))):
if not env2bool(env.DVCLIVE_HTML, "0"):
env_config["_report"] = None
else:
path = str(env_config["_path"])
Expand Down Expand Up @@ -196,9 +191,8 @@ def make_summary(self):
def make_report(self):
if self._report == "html":
html_report(self.dir, self.summary_path, self.html_path)
if self._auto_open:
if env2bool(env.DVCLIVE_OPEN):
Copy link
Copy Markdown

@dberenbaum dberenbaum Apr 22, 2022

Choose a reason for hiding this comment

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

What should the default be? Should we try to detect tty if no environment variable is set? Are there ever cases where the report was auto opened by default before?

Copy link
Copy Markdown
Contributor Author

@daavoo daavoo Apr 25, 2022

Choose a reason for hiding this comment

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

What should the default be?

It's currently False and I guess that's what should be

Should we try to detect tty if no environment variable is set?

I'm not sure if it's worth the effort. We provide a way for users to handle the option in different environments, let's be explicit in docs about how to use it.

Are there ever cases where the report was auto opened by default before?

Only if dvc config plots.auto_open was set to True.

Copy link
Copy Markdown

@dberenbaum dberenbaum Apr 25, 2022

Choose a reason for hiding this comment

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

My worry is that the point of auto open is to make the simplest use case (showing the live report) as easy as possible, and this creates an additional step. It almost seems pointless to have the environment variable, since it's mostly for people who don't read the docs or can't immediately figure out how to open the report.

Maybe it's not really an issue. I think it's fine to merge, but I'm curious about whether auto open is ever needed and if there's some way to make this scenario useful.

Copy link
Copy Markdown
Contributor Author

@daavoo daavoo Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good points.

If I remember correctly, requests for making it default False were coming from Vscode, not users.

Perhaps it makes sense to default to True, as we provide a way for external products (and users) to override the default behavior.

Copy link
Copy Markdown
Contributor Author

@daavoo daavoo Apr 26, 2022

Choose a reason for hiding this comment

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

@mattseddon Would it be okay for you if we make the default True? vscode would need to always set the DVCLIVE_OPEN var in order to prevent the behavior

Copy link
Copy Markdown

@dberenbaum dberenbaum Apr 26, 2022

Choose a reason for hiding this comment

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

I realized we have had this discussion before (multiple times 😅):

I don't think it's worth revisiting, so let's not open by default.

It might be enough to hint about the environment variable like in https://github.com/iterative/dvc/blob/f84f9cf1846554a13f388693220378edb8d17be6/dvc/commands/plots.py#L111.

Copy link
Copy Markdown

@dberenbaum dberenbaum Apr 27, 2022

Choose a reason for hiding this comment

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

It might be enough to hint about the environment variable like in https://github.com/iterative/dvc/blob/f84f9cf1846554a13f388693220378edb8d17be6/dvc/commands/plots.py#L111.

@daavoo What do you think about this idea?

open_file_in_browser(self.html_path)
self._auto_open = False

def read_step(self):
if Path(self.summary_path).exists():
Expand Down
23 changes: 23 additions & 0 deletions dvclive/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import base64
import csv
import os
import re
import webbrowser
from collections.abc import Mapping
from pathlib import Path
Expand Down Expand Up @@ -45,8 +47,29 @@ def to_base64_url(image_file):
return f"data:image;base64,{base64_str}"


def run_once(f):
def wrapper(*args, **kwargs):
if not wrapper.has_run:
wrapper.has_run = True
return f(*args, **kwargs)

wrapper.has_run = False
return wrapper


@run_once
def open_file_in_browser(file) -> bool:
path = Path(file)
url = path if "Microsoft" in uname().release else path.resolve().as_uri()

return webbrowser.open(url)


def env2bool(var, undefined=False):
"""
undefined: return value if env var is unset
"""
var = os.getenv(var, None)
if var is None:
return undefined
return bool(re.search("1|y|yes|true", var, flags=re.I))
11 changes: 7 additions & 4 deletions tests/test_report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
# pylint: disable=unused-argument
import os

import pytest
from PIL import Image

from dvclive import Live
from dvclive.data import Image as LiveImage
from dvclive.data import Scalar
from dvclive.data.plot import ConfusionMatrix, Plot
from dvclive.env import DVCLIVE_OPEN
from dvclive.report import (
get_image_renderers,
get_plot_renderers,
Expand Down Expand Up @@ -58,7 +60,8 @@ def test_get_renderers(tmp_dir, mocker):
assert plot_renderers[0].properties == ConfusionMatrix.get_properties()


def test_make_report_open(tmp_dir, mocker):
@pytest.mark.vscode
def test_make_report_open(tmp_dir, mocker, monkeypatch):
mocked_open = mocker.patch("webbrowser.open")
live = Live()
live.log_plot("confusion_matrix", [0, 0, 1, 1], [1, 0, 0, 1])
Expand All @@ -67,15 +70,15 @@ def test_make_report_open(tmp_dir, mocker):

assert not mocked_open.called

mocked_open = mocker.patch("webbrowser.open")
live = Live(report=None)
live.log("foo", 1)
live.next_step()

assert not mocked_open.called

mocked_open = mocker.patch("webbrowser.open")
live = Live(auto_open=True)
monkeypatch.setenv(DVCLIVE_OPEN, True)

live = Live()
live.log_plot("confusion_matrix", [0, 0, 1, 1], [1, 0, 0, 1])
live.make_report()

Expand Down