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

ref: rewrap a couple usage blocks #3345

Merged
merged 8 commits into from
Mar 30, 2022
Merged

ref: rewrap a couple usage blocks #3345

merged 8 commits into from
Mar 30, 2022

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Mar 9, 2022

Per #3325 (review) and #3325 (review) originally

Some quality-related ideas on what criteria to use when formatting usage blocks. Potentially we can later match this in the core repo too (-h help output).

@jorgeorpinel jorgeorpinel requested a review from daavoo March 9, 2022 08:14
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-refs-re-fgvayc March 9, 2022 08:14 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review March 9, 2022 08:26
@jorgeorpinel jorgeorpinel marked this pull request as draft March 9, 2022 08:26
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-dvclive-refs-re-fgvayc March 9, 2022 08:44 Inactive
@jorgeorpinel jorgeorpinel requested a review from shcheklein March 9, 2022 08:47
@jorgeorpinel jorgeorpinel added the type: discussion Requires active participation to reach a conclusion. label Mar 9, 2022
@jorgeorpinel jorgeorpinel requested a review from iesahin March 9, 2022 08:48
@jorgeorpinel jorgeorpinel force-pushed the dvclive/refs-rewrap branch from d131954 to 0491087 Compare March 9, 2022 08:51
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-dvclive-refs-re-fgvayc March 9, 2022 08:51 Inactive
@jorgeorpinel jorgeorpinel self-assigned this Mar 10, 2022
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Is there a way to handle this criteria in the auto-formatter/linter?

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Mar 15, 2022

Meaning the line length specific to code blocks? I don't think so unfortunately, in fact md code blocks are allowed any line width by the linter, I think. We should always sanity check the rendered review app before merging -- that's how I detect code blocks with scroll bars.

@jorgeorpinel jorgeorpinel marked this pull request as ready for review March 15, 2022 17:27
@jorgeorpinel jorgeorpinel requested review from dberenbaum and removed request for shcheklein March 15, 2022 17:27
usage: dvc repro [-h] [-q | -v] [-f] [-s] [-m] [--dry] [-i]
[-p] [-P] [-R] [--no-run-cache] [--force-downstream]
[--no-commit] [--downstream] [--pull] [--glob]
usage: dvc repro [-h] [-q | -v] [-f] [-s] [-m] [-i] [-p] [-P] [-R]
Copy link
Contributor

Choose a reason for hiding this comment

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

So this groups all the short flags at the start? Looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then there's additional reordering within some of the long flags. Can you give some rationale for the order @jorgeorpinel?

I'm not sure if there was any rationale for the current order. cc @iterative/dvc If you have any context for the order of arguments under these commands, please add your thoughts 🙏 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that on the core repo, currently new arguments are just added at the bottom so there's no logical order.

My criteria here isn't super defined but broadly: put general ones in the first line, esp. if they're shot flags; Then group by functionality in the order in which we think people will use them most -- in the case above (repro) realted to deps/params, then outs/metrics/plots, then DVCLive.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we move [--no-exec] [--no-run-cache] [--no-commit] to the bottom for run and stage add, can we put the related repro options [--pull] [--no-run-cache] [--no-commit] at the end here?

Copy link
Contributor

@daavoo daavoo Mar 25, 2022

Choose a reason for hiding this comment

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

  • Should the ordering criteria be translated back to dvc core repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, once there is an agreement on the order of everything, we should update the core repo.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Mar 29, 2022

Choose a reason for hiding this comment

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

move [--no-exec] [--no-run-cache] [--no-commit] to the bottom for run

Done. Well, I moved them above the DVCLive ones. PTAL

and stage add

Doesn't have them.

put the related repro options [--pull] [--no-run-cache] [--no-commit] at the end

Done.


  • BTW run --no-exec is equivalent to repro --dry right? May want to rename the former to "dry" as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • BTW run --no-exec is equivalent to repro --dry right? May want to rename the former to "dry" as well.

I think --dry is reserved for commands that are read-only and make no actual changes to the repo state, whereas run --no-exec is basically dvc stage add and still writes the stage to dvc.yaml.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Mar 31, 2022

Choose a reason for hiding this comment

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

repro is not read-only; It changes the repo state. So should its --dry be renamed --no-exec then? UPDATE: Moved to iterative/dvc#7522

[-m <path>] [-M <path>]
[--plots <path>] [--plots-no-cache <path>]
[--live <path>] [--live-no-cache <path>]
[--live-no-html] [-w <path>]
[--always-changed] [--external] [--desc <text>]
Copy link
Contributor

Choose a reason for hiding this comment

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

And these are stage-level options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Least used options

@iesahin
Copy link
Contributor

iesahin commented Mar 30, 2022

I approved this again but is this an epic?

@jorgeorpinel
Copy link
Contributor Author

is this an epic?

No but there are follow up tasks (I'll make an issue after this or something).

@jorgeorpinel jorgeorpinel requested a deployment to dvc-org-dvclive-refs-re-fgvayc March 30, 2022 21:43 Abandoned
@jorgeorpinel jorgeorpinel requested a deployment to dvc-org-dvclive-refs-re-fgvayc March 30, 2022 21:49 Abandoned
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-dvclive-refs-re-fgvayc March 30, 2022 21:52 Inactive
@jorgeorpinel jorgeorpinel merged commit a997ec2 into master Mar 30, 2022
@jorgeorpinel jorgeorpinel deleted the dvclive/refs-rewrap branch March 30, 2022 21:53
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* ref: rewrap a couple usage blocks
per #3325 (review)
and #3325 (review)

* ref: reorg repro/run/stage add usage block

* move run/repro exec modifyer options down
per #3345 (comment)

* Update content/docs/command-reference/repro.md

Co-authored-by: Dave Berenbaum <[email protected]>

* ref: make run options like stage add
per #3345 (review)

Co-authored-by: Dave Berenbaum <[email protected]>
@jorgeorpinel jorgeorpinel mentioned this pull request Sep 6, 2022
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference p2-nice-to-have Less of a priority at the moment. We don't usually deal with this immediately. labels Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference p2-nice-to-have Less of a priority at the moment. We don't usually deal with this immediately. type: discussion Requires active participation to reach a conclusion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants