-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add config option plots.auto_open
#7159
Conversation
plots.auto_open
and ENV var DVCLIVE_OPEN
plots.auto_open
and env var DVCLIVE_OPEN
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.
Just tried this out in CLI and VSCode and successfully didn't have a tab open! As long as the code is good on the DVC end, merging this will fix our issue.
Hi. Q: where will |
Not sure if It should be documented at all as It is intended for VSCode internal usage |
5643bdf
to
138c2a8
Compare
Thanks @daavoo for taking care of this quick! Should we name things in a similar way - DVC_PLOTS_AUTO_OPEN ? |
dvc/repo/live.py
Outdated
|
||
webbrowser_open(index_path) | ||
auto_open = out.repo.config["plots"].get("auto_open", False) | ||
if auto_open and int(os.environ.get(DVCLIVE_OPEN, "1")): |
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.
plots.auto_open
vs DVCLIVE_OPEN
are not symmetrical though. WDYT of DVC_PLOTS_OPEN
since we have plans to do that automatically (#6091)?
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.
Another thought: should we be more general and implement core.no_browser
/ui.no_browser
and DVC_NO_BROWSER
instead?
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 guess it's a thing to discuss in #6091 than here.
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.
The env var, in this case, was added for VSCode specifically, not really for end-user usage (however, it's not clear yet if they will need it or not). The asymmetry was intentional in order to allow VSCode to reactive DVCLIVE HTML even if user had plots.auto_open
enabled.
Regardless, if going for the general approach, is there a reason to have both a dvc config
field and an env var for the same purpose?
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.
We've decided on the VSCode end, at least for now, to respect the user's configuration for browser opening. Something like DVC_NO_BROWSER
could be useful in the future for us and/or other integrations should we get user requests for a VSCode extension feature to disable browser opening outside the config.
Having an env var alongside the config option could help us do so in a more temporary manner, but if dvc config is able to do the same with some sort of invocation option then the env var would be rendered redundant.
f24ea56
to
3387ac1
Compare
plots.auto_open
and env var DVCLIVE_OPEN
plots.auto_open
Option for enabling automatically open webbrowser with generated html. Defaults False. Closes #6091
3387ac1
to
4e5650d
Compare
β 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.
Closes #6091
Closes iterative/dvclive#201
dvc.org
P.R: iterative/dvc.org#3101