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

guide: external data updates #1735

Merged
merged 9 commits into from
Sep 1, 2020
Merged

guide: external data updates #1735

merged 9 commits into from
Sep 1, 2020

Conversation

jorgeorpinel
Copy link
Contributor

and other related updates

@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 28, 2020 17:44 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review August 28, 2020 19:31
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 28, 2020 19:32 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 28, 2020 21:15 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 30, 2020 05:24 Inactive
and reorder cache.{type} options to match std. remote order
per #1735 (review)
et al.
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 30, 2020 05:25 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 30, 2020 06:17 Inactive
@shcheklein
Copy link
Member

@jorgeorpinel what is the scope, goal of this PR? can we split it if there are many changes?

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.

set the scope please, split if needed into separate PR if changes are not related

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Aug 31, 2020

All the changes are related the way I see it @shcheklein . The goal was to update the external data docs: https://dvc.org/doc/user-guide/external-dependencies and https://dvc.org/doc/user-guide/managing-external-data which were incomplete and outdated. Theres 2 more files that have small updates directly related to the changes in the aforementioned x deps/outs docs:

Comment on lines 156 to -160
`dvc remote` for more information on "local remotes".) This will overwrite the
value provided to `dvc config cache.dir` or `dvc cache dir`.

- `cache.ssh` - name of an
[SSH remote to use as external cache](/doc/user-guide/managing-external-data#ssh).
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Aug 31, 2020

Choose a reason for hiding this comment

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

This file was just reordered to match the standard sorting of remote types (implemented in recent PRs) — but these cache.{type} options are directly related to external cache setup, and the docs link to each other.

<abbr>workspace</abbr>) are also supported.
<abbr>workspace</abbr>) are also supported (except metrics and plots).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of an important note added to the external outputs doc.


<details>

### Expand to see resulting DVC-file
### Expand to see resulting .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.

we can now use inline code

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Aug 31, 2020

Choose a reason for hiding this comment

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

Yes, thanks. But actually this may look better? In any case we have a few other similar headers in other docs so I can update them all in a separate small PR if you like how it looks with back quotes more. Lmk please

without .dvc file

with .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.

I don't know, I prefer them highlighted. It feels consistent.

Copy link
Contributor

@rogermparent rogermparent Aug 31, 2020

Choose a reason for hiding this comment

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

That PR that introduced code blocks in details also changed existing triggers that have "DVC-file" to ".dvc file".

Here's the details that are like this in master right now

content/docs/user-guide/external-dependencies.md
157:### Expand to see resulting .dvc file
196:### Expand to see resulting .dvc file

The remaining instances of "DVC-file" in headings only exist in the Heartbeat blog posts.

If we decide "DVC-file" looks better, we'll have to change them all back in a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK changed for this one file. Will send all the other instances in a separate PR when this one is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rogermparent "DVC-file" is a deprecated term 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake, I get what you mean now!

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq August 31, 2020 22:43 Inactive
@@ -112,16 +123,11 @@ Please check that you are able to connect both ways with tools like `ssh` and
### HDFS

```dvc
# Add HDFS remote to be used as cache location for HDFS files
Copy link
Member

Choose a reason for hiding this comment

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

should we put some summary of these comments above of after the code blocks?

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's all under the ## Examples header now as a numbered list 🙂

Maybe we should make all these H3s into expandable details sections so that you don't have to scroll that much between the numbered list and the actual example?

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.

Looks a bit aggressive on removing comments, otherwise good to go!

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-1wquguk7rhkq September 1, 2020 02:04 Inactive
@jorgeorpinel
Copy link
Contributor Author

Thanks Ivan! Well, I'm going to merge this but lmk about making details sections or a summary of the comments in each H3 (that's kind of repetitive though) and I'll add it to the next PR (coming up).

@jorgeorpinel jorgeorpinel merged commit 527ff87 into master Sep 1, 2020
jorgeorpinel added a commit that referenced this pull request Sep 1, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Sep 1, 2020
shcheklein pushed a commit that referenced this pull request Sep 4, 2020
* guide: use back quotes in headers
per #1735 (comment)

* guide: use ```yaml instead of yml (consistency)

* guide: capitalize bullet list

* intsall: remove unnecessary line in Linux/snap

* guide: make H3s into expandable sections in x data docs
per #1735 (comment)

* cmd: cosmetic updates to cache.{type} config option descriptions
shcheklein pushed a commit that referenced this pull request Sep 17, 2020
* guide: use back quotes in headers
per #1735 (comment)

* guide: use ```yaml instead of yml (consistency)

* guide: capitalize bullet list

* intsall: remove unnecessary line in Linux/snap

* guide: make H3s into expandable sections in x data docs
per #1735 (comment)

* cmd: cosmetic updates to cache.{type} config option descriptions

* cmd: std option descs in diff
per pending items in #1758

* cmd: small impro to diff

* term: review usage of SCM

* cmd: remove redundant wording in add

* cmd: fix format and typo intro by #1779
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.

4 participants