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.

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

exp init: add simple dvc exp init command #6621

Merged
merged 2 commits into from
Sep 16, 2021
Merged

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Sep 15, 2021

Simpler, initial implementation for exp init.

Closes #6441.
Part of #6446.

@skshetry skshetry added A: experiments Related to dvc exp A: cli Related to the CLI labels Sep 15, 2021
@skshetry skshetry self-assigned this Sep 15, 2021
@skshetry skshetry requested a review from a team as a code owner September 15, 2021 08:35
@skshetry skshetry added the feature is a feature label Sep 15, 2021
@skshetry skshetry force-pushed the exp-init branch 2 times, most recently from c93df39 to e795838 Compare September 15, 2021 08:49
metrics_no_cache=[metrics],
plots_no_cache=[plots],
live=dvclive,
force=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think force should probably be an included flag. Not sure it's a good idea to force by default.

Edit: It might be unclear to a new user that this may overwrite an existing stage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also a note that we obviously need to revisit this later to generalize for other templates.

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'll add the force option internally on the next PR. :)

dvc/command/experiments.py Outdated Show resolved Hide resolved
Comment on lines +828 to +829
_, ext = os.path.splitext(params_path)
params = list(LOADERS[ext](params_path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't need to block this PR, but see #6605 for proposal to eliminate need for parameter parsing here.

help="Prompt for values that are not provided",
)
experiments_init_parser.add_argument(
"--template", help="Stage template to use to fill with provided values"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should include the default value in the help message (and for all non-boolean options below).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add defaults in the upcoming PRs, as the place where I'll keep those defaults may constantly change. :)

dvc/command/experiments.py Outdated Show resolved Hide resolved
dvc/command/experiments.py Outdated Show resolved Hide resolved
params=[{params_path: params}],
metrics_no_cache=[metrics],
plots_no_cache=[plots],
live=dvclive,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default stage can ignore --dvclive arg. We probably need conditional arg parsing based on the template, throwing an error if a provided argument is irrelevant for that template.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a default stage/template yet, so I left it with the default. I'll change this when we have templates.

dvc/command/experiments.py Outdated Show resolved Hide resolved
@skshetry
Copy link
Member Author

@dberenbaum, I am merging this and will follow up on your comments in the future PRs. The command is hidden meanwhile.

@skshetry skshetry merged commit 978d5ac into iterative:master Sep 16, 2021
@skshetry skshetry deleted the exp-init branch September 16, 2021 10:36
"--template", help="Stage template to use to fill with provided values"
)
experiments_init_parser.add_argument(
"--explicit", help="Only use the path values explicitly provided"
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, feel free to suggest a better name for this option. Maybe --ignore-missing?

@skshetry skshetry added the skip-changelog Skips changelog label Sep 17, 2021
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 feature is a feature skip-changelog Skips changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exp init: create command to append a default experiment stage
2 participants