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

Update dvc root command reference motivation and examples #1637

Merged
merged 19 commits into from
Jul 31, 2020

Conversation

jeremydesroches
Copy link
Contributor

@jeremydesroches jeremydesroches commented Jul 29, 2020

Updates motivation and examples. See issue #404.

@jeremydesroches jeremydesroches changed the title Clarifies motivation and description, adds relevant examples Clarifies dvc root motivation and description, adds relevant examples Jul 29, 2020
@jeremydesroches jeremydesroches changed the title Clarifies dvc root motivation and description, adds relevant examples Fix #404: Clarifies dvc root motivation and description, adds relevant examples Jul 29, 2020
@jorgeorpinel
Copy link
Contributor

please double-check my formatting and style

That's checked automatically by Github and Restyled (shown in the bottom PR summary). You can see all the checks in that section of the PR (inside "test", specifically yarn format-check). These should also be run locally automatically if you setup the dev env as explained in our docs contrib guide. Please lmk i fyou have questions about any of that.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Hey Jeremy. Thanks for the PR.

I haven't checked the examples yet but the description didn't fully convince me.

I left specific comments ☝️

@jeremydesroches
Copy link
Contributor Author

Thanks Jorge. Made changes based on your feedback and left comments open.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Few more questions on the description. Thanks

content/docs/command-reference/root.md Show resolved Hide resolved
content/docs/command-reference/root.md Outdated Show resolved Hide resolved
content/docs/command-reference/root.md Outdated Show resolved Hide resolved
@jorgeorpinel
Copy link
Contributor

@jeremydesroches please check #404 (comment) before continuing here 🙏

@jeremydesroches jeremydesroches changed the title Fix #404: Clarifies dvc root motivation and description, adds relevant examples Update dvc root command reference motivation and description, add examples Jul 30, 2020
@jorgeorpinel jorgeorpinel changed the title Update dvc root command reference motivation and description, add examples Update dvc root command reference motivation and description, review examples Jul 30, 2020
@jorgeorpinel jorgeorpinel changed the title Update dvc root command reference motivation and description, review examples Update dvc root command reference motivation and examples Jul 30, 2020
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

I noticed you added Examples @jeremydesroches. Thanks, I'll review them. But please first make sure the old ones (first 2) are "actionable and meaningful. It's should be easy to run commands to see the results", per #404

@jeremydesroches
Copy link
Contributor Author

I noticed you added Examples @jeremydesroches. Thanks, I'll review them. But please first make sure the old ones (first 2) are "actionable and meaningful. It's should be easy to run commands to see the results", per #404

I reviewed the existing two examples before adding the new ones.

The first example is still meaningful and actionable because it's important to show that dvc root returns a relative reference when and where it's run.

I adjusted the second example to make it more a meaningful and useful (while keeping it simple). The updated version uses dvc root to specify the output of a dvc get command, while working from a subdirectory.

@jorgeorpinel

This comment has been minimized.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Left some more comments above ☝️

And a review of the Examples below 👇

content/docs/command-reference/root.md Outdated Show resolved Hide resolved
content/docs/command-reference/root.md Outdated Show resolved Hide resolved
content/docs/command-reference/root.md Outdated Show resolved Hide resolved
content/docs/command-reference/root.md Outdated Show resolved Hide resolved
Comment on lines 83 to 92
## Example: Build reusable paths

Build reusable paths to dependencies, scripts, or <abbr>data artifacts</abbr>
from separate stages and subdirectories.

```dvc
$ cd more_stages/
$ dvc run -n process_data \
-d data.in \
-d $(dvc root)/process_data.py \
Copy link
Contributor

Choose a reason for hiding this comment

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

OK... I think what we need is a shell script that generates a dvc.yaml file instead of another dvc run example. Not required for this PR, so if you're not so familiar with Linux shell just leave it out for now please. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this for now.

I looked into some related issues (#615) and examples of scripts.

These make sense within the scripting use case but instead of adding a (complicated) example that builds out a full set of stages, it would help to have the team vet a smaller example that fits the recommended usage.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 03:20 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 04:13 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:01 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:23 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:31 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:31 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:35 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:35 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

OK this is ready I think. I'll just commit my couple pending suggestions and merge this.

@jorgeorpinel jorgeorpinel requested a deployment to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:39 Abandoned
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmdroot-de5w9at1p1 July 31, 2020 05:39 Inactive
@jorgeorpinel jorgeorpinel merged commit cb5f4d6 into iterative:master Jul 31, 2020
@jeremydesroches jeremydesroches deleted the cmdroot branch July 31, 2020 08:24
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.

3 participants