run/repro/stage add: regroup options/flags

[jorgeorpinel]

Matching treeverse/dvc.org#3345

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[dberenbaum]

Thanks for making this PR @jorgeorpinel! Besides the comment above, it looks like there are still some other arguments that don't match treeverse/dvc.org#3345. Could you take a look? I'm also happy to take this one over if you want.

[jorgeorpinel]

Sure, I'll take a look.

[jorgeorpinel]

@dberenbaum UPDATE: I'm actually trying to focus on other things. I don't mean to abandon this: I still plan to address the feedback ASAP. But feel free to take it over as well. I'll resolve the current merge conflict for now...

[jorgeorpinel]

Answer to #7524 (comment):

we should also have updated exp run in treeverse/dvc.org#3345 for consistency

Do we mean the options after the { repro options ... } line?

Is that missing -n/--name BTW?

We can update it here first and then make a follow-up to docs PR (maybe necessary anyway if the repro output is re-ordered again in this PR). Here's my suggestion:

usage: dvc exp run [-h] [-q | -v] [-f]
               { repro options ... } [-n <name>]  # adds --name
               [-S [<filename>:]<params_list>]  # on-the-fly params
               [--queue] [--run-all] [-j <number>] [--temp]  # parallel
               [-r <experiment_rev>] [--reset]  # for checkpoints
               [targets [targets ...]]

[jorgeorpinel]

there are still some other arguments that don't match treeverse/dvc.org#3345

I can't find any missing arguments in docs other than the ones surpassed in code @dberenbaum .

[jorgeorpinel]

BTW shouldn't -n|--name be added in _add_common_args()? It's a common arg both in stage add and run. Cc @karajan1001

[jorgeorpinel]

Also (💅🏼) should add_arguments() in repro/exp run use a similar private method name instead? _add_common_args() as well, even

[karajan1001]

I think we can

BTW shouldn't -n|--name be added in _add_common_args()? It's a common arg both in stage add and run. Cc @karajan1001

I think we can do that.

Also (💅🏼) should add_arguments() in repro/exp run use a similar private method name instead? _add_common_args() as well, even

I'm not quite understand here why this method to be a private method as it is used outside its file's scope.

[dberenbaum]

BTW shouldn't -n|--name be added in _add_common_args()? It's a common arg both in stage add and run. Cc @karajan1001

Resolved.

[dberenbaum]

Never mind, rolling this change back. It breaks a bunch of tests. I think they were intentionally left separate to support some behavior that was deprecated but not actually removed (like dvc run --single-stage), where --name was not required in dvc run. Let's stick with the UI changes.

[dberenbaum]

@jorgeorpinel @karajan1001 Could you please review the latest updates?

[jorgeorpinel]

Well, run is not exactly like in docs because of the way it's coded but I think we agreed to let that go. Everything else does match treeverse/dvc.org#3345 so please merge!

[dberenbaum]

Well, run is not exactly like in docs because of the way it's coded but I think we agreed to let that go. Everything else does match iterative/dvc.org#3345 so please merge!

Thanks, @jorgeorpinel! Based on the comments in this PR, we updated the docs in treeverse/dvc.org#3435, and they should all match now AFAIK.