Skip to content
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

Jump to bottom

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

Merged
merged 2 commits into from
Oct 2, 2021
Merged

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

merged 2 commits into from
Oct 2, 2021

Conversation

skshetry
Copy link
Member

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
skshetry
skshetry commented Sep 30, 2021
View reviewed changes
dvc/command/experiments.py Outdated

if not live:
ui.error_write()
live = Confirm.ask(
Copy link
Member Author

@skshetry skshetry Sep 30, 2021 •
edited
Loading

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
Collaborator

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.

skshetry
skshetry commented Sep 30, 2021
View reviewed changes
dvc/command/experiments.py Outdated
@@ -1548,6 +1592,13 @@ def add_parser(subparsers, parent_parser):
experiments_init_parser.add_argument(
"--live", help="Path to log dvclive outputs for your experiments"
)
experiments_init_parser.add_argument(
"--with",
Copy link
Member 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
Collaborator

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
Member 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.

@skshetry
exp init: introduce --with={default,live} flag
6739f1c 
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 reviewed Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@dberenbaum dberenbaum left a comment

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?

dvc/command/experiments.py Outdated
@@ -1548,6 +1592,13 @@ def add_parser(subparsers, parent_parser):
experiments_init_parser.add_argument(
"--live", help="Path to log dvclive outputs for your experiments"
)
experiments_init_parser.add_argument(
"--with",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

dvc/command/experiments.py Outdated

if not live:
ui.error_write()
live = Confirm.ask(
Copy link
Collaborator

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.

dvc/command/experiments.py
{
"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
Collaborator

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
Member 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. 😄

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
Collaborator

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
Copy link
Member Author

skshetry commented Oct 1, 2021

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 approved these changes Oct 1, 2021
View reviewed changes
Copy link
Collaborator

@dberenbaum dberenbaum left a comment

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 iterative:master Oct 2, 2021
@skshetry skshetry deleted the exp-with branch October 2, 2021 10:27
@iesahin iesahin mentioned this pull request Oct 27, 2021
Closed
29 tasks
@skshetry skshetry mentioned this pull request Apr 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dberenbaum dberenbaum dberenbaum approved these changes

Assignees

@skshetry skshetry

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skshetry @dberenbaum