Skip to content

exp init: introduce --with={default,live} flag - #6716

Merged
skshetry merged 2 commits into
treeverse:masterfrom
skshetry:exp-with
Oct 2, 2021
Merged

exp init: introduce --with={default,live} flag#6716
skshetry merged 2 commits into
treeverse:masterfrom
skshetry:exp-with

Conversation

@skshetry

Copy link
Copy Markdown
Collaborator

This flag will select the different mode of outputs of stage.
With --with=live, it'll enable live setting in the stage.
On interactive mode, it'll always ask user if they want to
use live (unless --with=live is specified), and then it'll
skip metrics/plots question and moves on to ask for path to
dvclive path.

Also, if live setting is enabled in the stage, it'll set the
models to be a checkpoint output.

@skshetry skshetry added ui user interface / interaction A: experiments Related to dvc exp A: cli Related to the CLI A: pipelines Related to the pipelines feature labels Sep 30, 2021
@skshetry skshetry self-assigned this Sep 30, 2021
@skshetry
skshetry requested a review from a team as a code owner September 30, 2021 10:41
Comment thread dvc/command/experiments.py Outdated

@skshetry skshetry Sep 30, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will need to iterate on prompts separately.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, we don't need to block this PR, but how about, "Do you want to log checkpoints with dvclive"? We should probably make clear that this will generate checkpoints. Also, we will maybe want to link to dvclive docs.

Comment thread dvc/command/experiments.py Outdated

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not so sure about this flag, can change to anything better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--type? --format? --stage?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I prefer --type, --stage has a different meaning. Compared to --format, --type is more simpler.

This flag will select the different mode of outputs of stage.
With `--with=live`, it'll enable `live` setting in the stage.
On interactive mode, it'll always ask user if they want to
use live (unless `--with=live` is specified), and then it'll
skip metrics/plots question and moves on to ask for path to
dvclive path.

Also, if live setting is enabled in the stage, it'll set the
`models` to be a checkpoint output.
@skshetry
skshetry requested review from dberenbaum and removed request for isidentical September 30, 2021 10:43
@skshetry skshetry changed the title exp init: introduce --with={default,live} flag [WIP] exp init: introduce --with={default,live} flag Sep 30, 2021

@dberenbaum dberenbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels a little clunky to me to ask about dvclive in the middle of entering paths. I think it's better to require --with non-interactively (--name, --run, etc. aren't covered). It also fits with the description of --interactive: Prompt for values that are not provided.

Minor note: should --template be removed at this point?

Comment thread dvc/command/experiments.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--type? --format? --stage?

Comment thread dvc/command/experiments.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, we don't need to block this PR, but how about, "Do you want to log checkpoints with dvclive"? We should probably make clear that this will generate checkpoints. Also, we will maybe want to link to dvclive docs.

{
"cmd": "[b]Command[/b] to execute",
"code": "Path to a [b]code[/b] file/directory",
"data": "Path to a [b]data[/b] file/directory",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not include params here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to separate dependencies and then pipeline artifacts, but I don't need to. 😄

Comment thread dvc/command/experiments.py Outdated
if not live:
ui.error_write()
live = Confirm.ask(
"Do you want to log training steps iteratively with "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this makes sense for dvc exp init -i --with=default.

@skshetry

skshetry commented Oct 1, 2021

Copy link
Copy Markdown
Collaborator Author

It feels a little clunky to me to ask about dvclive in the middle of entering paths. I think it's better to require --with non-interactively (--name, --run, etc. aren't covered).

I felt the same way, I thought it was just me. I'll make it a requirement then. :)

@dberenbaum dberenbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

My only concern is that we may need to come back to the idea of custom templates again in the future. For example, users who mainly plot image files might want plots with cache: true. We don't want to turn this into dvc stage add, but it seems useful to enable a consistent stage structure, especially if it could be shared across projects. This seems like it might require some redesign if we ever need it, but for now it's just a note to keep in mind.

@skshetry skshetry changed the title [WIP] exp init: introduce --with={default,live} flag exp init: introduce --with={default,live} flag Oct 2, 2021
@skshetry
skshetry merged commit e58fcd8 into treeverse:master Oct 2, 2021
@skshetry
skshetry deleted the exp-with branch October 2, 2021 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: cli Related to the CLI A: experiments Related to dvc exp A: pipelines Related to the pipelines feature ui user interface / interaction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants