-
Notifications
You must be signed in to change notification settings - Fork 397
metrics: info on non-output usage #3387
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
Conversation
> - even though `dvc metrics` subcommands use the metrics files specifed in | ||
> `dvc.yaml` by default, they can be used on any file, as long as they are | ||
> specified via `targets` argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm. I see. I think instead of changing this note we should incorporate the information to the paragraph before the summary.json
sample block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e. starting in line 50 change
`dvc metrics` subcommands by default use the metrics files specified in
`dvc.yaml` ...
to something like
`dvc metrics` subcommands can be used on any
[valid metrics files](#supported-file-formats). By default they use the
ones listed in `dvc.yaml` ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I see that this info. is already in https://dvc.org/doc/command-reference/metrics/show and diff 🙂 (around targets
):
So please just address the comment above so we can wrap this up. Thanks
`dvc.yaml` (if any), for example `summary.json` below: | ||
`dvc metrics` subcommands can be used on any | ||
[valid metrics files](#supported-file-formats). By default they use the metrics | ||
files specified in `dvc.yaml` (if there are any), for example `summary.json` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed if any
tp if there are any
- sounded better to me, but it might be just me, I can roll this back :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks. This works perfectly in terms of meaning. I'll rephrase the wording a little for readability.
Co-authored-by: Paweł Redzyński <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @pared !
* metrics: info on non-output usage Fixes: #1909 * Update content/docs/command-reference/metrics/index.md * Update content/docs/command-reference/metrics/show.md Co-authored-by: Paweł Redzyński <[email protected]> * Restyled by prettier (#3412) Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Jorge Orpinel <[email protected]> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]>
Per iterative/dvc#5339
Closes: #1909