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

docs: add desc field #1951

Merged
merged 1 commit into from
Nov 26, 2020
Merged

docs: add desc field #1951

merged 1 commit into from
Nov 26, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Nov 17, 2020

Per iterative/dvc#4901

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

@shcheklein shcheklein temporarily deployed to dvc-landing-fix-dvc-490-bznorm November 17, 2020 01:01 Inactive
@@ -148,6 +148,8 @@ directory symlinks.
pattern as specified by `target`. Shell-style wildcards are supported: `*`,
`?`, `[seq]`, `[!seq]`, and `**`.

- `--desc` - arbitrary string description for generated `.dvc` file.
Copy link
Member

Choose a reason for hiding this comment

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

for generated .dvc file -> for data artifact ... or something similar, it does not describe .dvc file.

I wonder if we should provide a short-cut (-d)? also, would -m be a better option? to remind git commit -m ?

Copy link
Member

Choose a reason for hiding this comment

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

it should be also --desc <description> I think?

Copy link
Contributor Author

@efiop efiop Nov 17, 2020

Choose a reason for hiding this comment

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

for generated .dvc file -> for data artifact ... or something similar, it does not describe .dvc file.

Depends on how to look at it. It is not really for specific out either, from the internal perspective. In 2.0 we'll likely simplify the internal structure for dvc add-generated file, and then your suggestion will be 100% correct.

I wonder if we should provide a short-cut (-d)? also, would -m be a better option? to remind git commit -m ?

Both -d and -m are taken. Don't want to take up short options for it just yet. Also analogy with git commit -m is not that intuitive for this, IMO.

it should be also --desc I think?

Added. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Depends on how to look at it.

it's important to understand what is the purpose of this from the end user perspective and describe it in a way that would resonates with that in the first place. To my mind it's about providing descriptions to data unless I'm missing something. Now I read as I describe the file - why would I even want to do that?

Don't want to take up short options for it just yet.

yep, fine 👍

Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 17, 2020

Choose a reason for hiding this comment

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

To my mind it's about providing descriptions to data

Agree. There's already the meta field for general user info about the .dvc file itself.

would -m be a better option? to remind git commit -m

Good idea but I also think it's not intuitive: It could be confusing since this is add, not commit.

Both -d and -m are taken.

Not for add. Ah but they are fun run which also gets this new option, I see. Never mind.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 17, 2020

Choose a reason for hiding this comment

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

My suggestion (for now):

Suggested change
- `--desc` - arbitrary string description for generated `.dvc` file.
- `--desc <text>` - arbitrary description for the data `targets`. This doesn't
affect any DVC operations.

Please confirm metavar per iterative/dvc#4901 (review))

BTW does the same desc get applied to multiple targets (if several are provided to dvc add)? Thanks

Copy link
Contributor Author

@efiop efiop Nov 18, 2020

Choose a reason for hiding this comment

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

Thanks, updated the doc and the help in the original PR.

BTW does the same desc get applied to multiple targets (if several are provided to dvc add)? Thanks

Yes, it does.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

A few comments.

How about desc for imports/import-url?

Also, for outputs in the pipeline?

@shcheklein shcheklein temporarily deployed to dvc-landing-fix-dvc-490-bznorm November 17, 2020 01:14 Inactive
@efiop efiop changed the title docs: add desc field [WIP] docs: add desc field Nov 17, 2020
@efiop
Copy link
Contributor Author

efiop commented Nov 17, 2020

How about desc for imports/import-url?

Will add for symmetry. 👍

Also, for outputs in the pipeline?

I don't think it was part of the plan. But let me clarify...

Comment on lines 61 to 64
- `meta` (optional): Arbitrary metadata can be added manually with this field.
Any YAML contents is supported. `meta` contents are ignored by DVC, but they
can be meaningful for user processes that read `.dvc` files.
- `desc` (optional): Arbitrary string description for this `.dvc` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, so this field is for the whole .dvc file? So what's the difference with meta? I thought this would be per output (so, in the next bullet list).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both. meta is not used by us at all, it is a completely arbitrary metadata. desc is required to be a string/text and will be used by Viewer and maybe even dvc at some point.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 30, 2020

Choose a reason for hiding this comment

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

I see, thanks for the explanation @efiop. Would it be better to keep it hidden (undocumented) for now? I see no significative difference between meta and desc from the DVC user perspective. Possible source of confusion

p.s. I'd even suggest the Viewed uses a special format in meta values instead cc @fabiosantoscode maybe? For consideration

Copy link
Contributor

@fabiosantoscode fabiosantoscode Nov 30, 2020

Choose a reason for hiding this comment

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

Hey, I didn't know about this but it sounds like a good idea. Regarding screen real estate we can only really display it when you hover or click a file version, where we are going to show some more long form information like the full hash, delta and change dates.

@shcheklein what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've talked about meta before. meta is for arbitrary data that is not for us at all. Not even for viewer. desc is something that we recognize and something that viewer will be using very soon. We will be using it in some dvc commands in the future as well.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 1, 2020

Choose a reason for hiding this comment

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

desc is something that we recognize

But https://dvc.org/doc/user-guide/dvc-files-and-directories reads "This doesn't affect any DVC operations." so it's the Viewer that recognizes it (and to my mind it could do the same with a special format in meta but up to you guys). I suspect at some point the Viewer may also display meta for the user, so the distinction becomes blurry again.

My main objection is to have both these options documented since they look like the same from the user perspective (and desc is a little inconsistent as mentioned in #1951 (review) below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel It doesn't affect any dvc operations right now, but we still recognize it as a stage/output description string. Meta is also recognized by us but as an arbitrary object. Viewer will likely show it, but dvc - likely not. Still, those are separate things. We've considered using meta for it and decided not to.

Copy link
Contributor

@fabiosantoscode fabiosantoscode Dec 2, 2020

Choose a reason for hiding this comment

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

@shcheklein Not at all! I didn't know about this and it wasn't present in the designs

Copy link
Contributor

Choose a reason for hiding this comment

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

but we still recognize it as a stage/output description string

What does it mean to "recognize it"? Does DVC actually read/validate/process it in some way it doesn't do with meta? I do realize add/run have the --desc flag that writes it (except for stage outputs) so if that's what we mean then OK.

We've considered using meta for it and decided not to.

Sure, I'm not insisting. I'm just detecting possible inconsistencies and confusions. Maybe I'll just extract this to an issue so we can officially burry it as a nice-to-have 😋

Thanks for all the answers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does it mean to "recognize it"? Does DVC actually read/validate/process it in some way it doesn't do with meta? I do realize add/run have the --desc flag that writes it (except for stage outputs) so if that's what we mean then OK.

Yes, it does read/validate it as str. Meta is arbitrary, we don't validate it ever.

@shcheklein shcheklein temporarily deployed to dvc-landing-fix-dvc-490-bznorm November 18, 2020 19:03 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-fix-dvc-490-bznorm November 18, 2020 19:08 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-fix-dvc-490-bznorm November 18, 2020 19:17 Inactive
@efiop efiop changed the title [WIP] docs: add desc field docs: add desc field Nov 18, 2020
@efiop
Copy link
Contributor Author

efiop commented Nov 18, 2020

Combined your suggestions in both PRs @jorgeorpinel . Thanks!

@efiop efiop merged commit 4038814 into master Nov 26, 2020
@efiop efiop deleted the fix-dvc-4901 branch November 26, 2020 07:03
Comment on lines 76 to +79
- `cache`: Whether or not this file or directory is <abbr>cached</abbr> (`true`
by default, if not present). See the `--no-commit` option of `dvc add`.
- `desc`: User description for this output. This doesn't affect any DVC
operations.
Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 30, 2020

Choose a reason for hiding this comment

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

Also, is it correct that for .dvc files desc is a subfield inside outs? Seems odd since it's a top-level field in dvc.yaml files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't get that. I guess you're saying that yes, this is correct: desc is at the outs level in .dvc files, but at the top-level for dvc.yaml. This "inconsistency" seems unintuitive to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

desc can be for stage or for specific output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel Let's not focus on this too much, this is minor for docs.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 3, 2020

Choose a reason for hiding this comment

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

desc may be minor but 1. that doesn't mean its OK to have an error in docs if we see it (not the case here but I wasn't sure until now) and 2. this is making me realize the way we present outputs vs dvc.yaml in https://dvc.org/doc/user-guide/dvc-files-and-directories is confusing because outputs are only expanded on in the .dvc file section... which is kind of major. I'll address that elsewhere.

@jorgeorpinel
Copy link
Contributor

One last comment from me here: The dvc.yaml schema doesn't seem to have the field yet cc @skshetry thanks

See https://github.com/iterative/dvcyaml-schema/blob/master/schema.json

@skshetry
Copy link
Member

skshetry commented Dec 4, 2020

Thanks @jorgeorpinel for reminding. I'm planning to add them during 2.0 releases. I'm hoping if I can generate it automatically.

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.

5 participants