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 run: Separate exp param parsing from other commands #5310

Merged
merged 3 commits into from
Feb 16, 2021

Conversation

sjawhar
Copy link
Contributor

@sjawhar sjawhar commented Jan 21, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Purpose

Will close #5302

Approach

Since dvc exp run is the only command that uses parameter overrides (as compared with dvc run, where you are simply specifying a list of parameters), it makes sense for it have a separate parsing function. Conveniently, dvc.utils.cli_parse.loads_params is only being used by dvc.repo.experiments.run.run. So I renamed this function to loads_param_overrides for clarity and updated it to use the logic described in the source issue. I also updated the test to reflect the new possibilities.

I didn't see anywhere in the docs where dvc experiments is documented, otherwise I would open a PR there as well.

@pmrowla pmrowla self-requested a review January 23, 2021 01:25
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

Regarding the docs, there's no live documentation for experiments yet since 2.0 is still unreleased. There's an open docs PR that I can update once this is merged.

.gitignore Outdated Show resolved Hide resolved
@pmrowla pmrowla requested a review from dberenbaum February 1, 2021 06:25
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! Thanks, @sjawhar

@pmrowla pmrowla changed the title Separate exp param parsing from other commands exp run: Separate exp param parsing from other commands Feb 8, 2021
@sjawhar sjawhar force-pushed the feature/exp-params-cli branch from 7bb1a94 to d147bd6 Compare February 14, 2021 13:06
@pmrowla pmrowla self-requested a review February 14, 2021 14:27
@pmrowla pmrowla merged commit 66f24b8 into iterative:master Feb 16, 2021
@pmrowla
Copy link
Contributor

pmrowla commented Feb 16, 2021

Thanks @sjawhar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experiments, parameters, and the CLI: remove comma-separated parsing
3 participants