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

DVC File guide reorg/rewrite #2098

Merged
merged 37 commits into from
Jan 27, 2021
Merged

DVC File guide reorg/rewrite #2098

merged 37 commits into from
Jan 27, 2021

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jan 16, 2021

Closes #2059

Summarizing goals and principles here:

  1. Come up with a consistent structure for all DVC file refs, try to separate the guides from the refs.

  2. Write a proper guide for (basic) writing dvc.yaml (good story). Possible structure: dvc.yaml intro guide -> stage definition (ref+guide) -> "parametrization" -> "loops". But what about output entries and dvc.lock?

  3. Structure: From the research above, looks like a single long page that starts as mostly guide and ends as mostly ref, with as many sections as needed (long right-side ToC) is the most effective. We can always separate the ref. into a different schema ref. section later if needed (criteria: if we consider the dvc.yaml "language" central to DVC). Separately, we should try to improve the UI/design for a boost. See guide: make proper DVC File guide + separate reference? #2059 (comment).

  • Double check all redirects and links are finished before merging this.

@shcheklein shcheklein temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 03:26 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 03:44 Inactive
@jorgeorpinel jorgeorpinel changed the title [WIP] dvc.yaml guide reorg/rewrite [WIP] DVC File guide reorg/rewrite Jan 16, 2021
@shcheklein

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 04:06 Inactive
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 04:11 Inactive
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Jan 16, 2021

BTW I wouldn't even look at the diff in this PR, except maybe the list of files (all collapsed). Best to check the review app

@shcheklein

This comment has been minimized.

@shcheklein
Copy link
Member

I still don't the low level titles in the User Guide

DVC File is too generic and doesn't give any sense what is it about (also like you mentioned can include a lot of other stuff)

dvc.yaml - is too level, also doesn't mean anything. Except if we consider this a ref. Then it should be very dry, very formal, can be indeed in some place with all other files. Then we'll need a proper guide that is friendly.

Why don't we make dvc.yaml a proper section "Pipelines"? or something like that? For now it can be a single page, after if it becomes long, or we see other sections (e.g. Quick overview, Creating and updating, Parametrizing (Templating), Language Reference, etc)

There can be a section in user guide - Data Management (which might include .dvc) ,etc ..

It's one of the ideas of the structure that will be user friendly, will repeat Get Started to some extent which should make it easier to navigate. I guess there could be other approaches to this.

@shcheklein

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Jan 16, 2021

Why don't we make dvc.yaml a proper section "Pipelines"? For now it can be a single page

Good idea @shcheklein, maybe "Creating Pipelines"? We'll need a similar name for the .dvc file guide, perhaps "Tracking Existing Data"?

One problem though, is where to redirect https://dvc.org/doc/user-guide/dvc-files to, since that currently has an index that lists the kinds of DVC Files. I'll also have to review 32 existing links to /doc/user-guide/dvc-files throughout docs... (let's just redirect for now).

Also, so we won't have the "DVC FIle" concept around anymore? Or should it still be the term to unify .dvc, dvc.yaml, and dvc.lock (in the Basic Concepts section — not worked on in this PR but BTW that could be the final place to redirect the current DVC File guide index to).

Except if we consider this a ref. Then it should be very dry, very formal

I'm trying to avoid having another stand-alone ref. per the conclusions from #2059 (comment).

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 06:41 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 07:02 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 16, 2021 07:38 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 17, 2021 01:37 Inactive
@jorgeorpinel
Copy link
Contributor Author

Should we include "available in 2.0" note into some parts of the dvc.yaml doc?

@shcheklein depends on when this gets merged. I'd rather keep it out for now and add it last minute if needed.

@shcheklein
Copy link
Member

@jorgeorpinel I thought we wanted to merge and iterate fast (and thus avoiding redirects and stuff). There are not major problems in this iteration from what I see.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 03:08 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 03:08 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 03:13 Inactive
@jorgeorpinel jorgeorpinel changed the title [WIP] DVC File guide reorg/rewrite DVC File guide reorg/rewrite Jan 26, 2021
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 04:17 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 04:21 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 04:23 Inactive
@jorgeorpinel
Copy link
Contributor Author

OK @shcheklein, added the 2.0 warning (and finished addressing your other comments). Does that mean you approve this? 😏

Comment on lines 165 to 166
> Their paths will be evaluated based on
> [`wdir`](/doc/user-guide/dvc-files/dvc.yaml#accepted-fields), if one given.
Copy link
Member

Choose a reason for hiding this comment

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

This note should be moved on "Stage-specific values" 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.

[note on wdir] should be moved on "Stage-specific values

You mean also mentioned in that part? Because it's important here too (and this part comes first). For now I made it more general so people can assume it applies to local vars too.


### Parameter dependencies

[Parameters](/doc/command-reference/params) are a special type of stage
Copy link
Member

Choose a reason for hiding this comment

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

There's no non-default case? Was it documented before?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 26, 2021

Choose a reason for hiding this comment

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

There's no non-default [params] case?

Sorry, Idk what you mean @skshetry.

Params was never explained in the dvc.yaml guide before (only mentioned the params fields).

- ${model.filename}
```

⚠️ Known limitations of local `vars`:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this warning is useful as both of them are expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. Why would users expect or assume any of this? The one about wdir is a logical consequence of the loading order but that's a complex thing to grasp (for example it wasn't obvious to me at first and I had to ask). The one about foreach has some internal reason (I don't even remember it now) pretty much imposible for a user to know.

Plus we have all sorts of usage warnings throughout docs. I see this in line with that.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 10:56 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 26, 2021 20:51 Inactive
@jorgeorpinel jorgeorpinel had a problem deploying to dvc-landing-2-0-pipelin-rttezd January 26, 2021 20:56 Failure
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Jan 27, 2021

OK guys, I'm merging this since it's approved and I've gotten no further replies. But feel free to comment more: I can address anything left in another PR.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-2-0-pipelin-rttezd January 27, 2021 03:37 Inactive
@jorgeorpinel jorgeorpinel merged commit d9ce9a3 into master Jan 27, 2021
jorgeorpinel added a commit that referenced this pull request Jan 27, 2021
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) p1-important Active priorities to deal within next sprints C: guide Content of /doc/user-guide 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: guide Content of /doc/user-guide p1-important Active priorities to deal within next sprints
Projects
None yet
Development

Successfully merging this pull request may close these issues.

guide: make proper DVC File guide + separate reference?
4 participants