Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
I realized we have had this discussion before (multiple times 😅):
- plots: support opening plots file directly in the browser dvc#5584
- plots diff/show: configure --open flag dvc#6091
dvc: Option to automatically open the html report #180dvc: Add option to disable automatic open #201
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.
There was a problem hiding this comment.
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?
Remove `auto_open` arg. Add DVCLIVE_OPEN env var. Add env2bool utils. Closes #241
3 participants
Remove
auto_openarg.Add DVCLIVE_OPEN env var.
Add env2bool utils.
Closes #241