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: external: add a disclaimer #2104

Merged
merged 4 commits into from
Feb 1, 2021
Merged

docs: external: add a disclaimer #2104

merged 4 commits into from
Feb 1, 2021

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Jan 19, 2021

People find this article quite often and tend to misuse it. Added a simple disclaimer to try to redirect them.

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-efiop-patch-eu1luq January 19, 2021 20:58 Inactive
@shcheklein shcheklein had a problem deploying to dvc-landing-efiop-patch-eu1luq January 19, 2021 21:04 Failure
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-efiop-patch-eu1luq January 20, 2021 02:13 Inactive
Comment on lines 3 to 6
> ⚠️ Artifacts added with --external are not affected by
> `dvc push/pull/status -c`. You are likely looking for
> [straight-to-remote/cache](https://github.com/iterative/dvc/issues/4520)
> functionality or `dvc import-url`
Copy link
Contributor

Choose a reason for hiding this comment

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

Good note! If it's not urgent should we make this part of #2091 (review) ?

And prob. link to /doc/command-reference/add#transferring-data-directly-to-the-remote instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 will have to wait for 2.0 anyways, and even then people might try to use this. Better to have this note anyways.

@@ -1,7 +1,7 @@
# Managing External Data

> ⚠️ Artifacts added with --external are not affected by dvc push/pull/status
> -c. You are likely looking for
> ⚠️ Artifacts added with --external are not affected by
Copy link
Member

Choose a reason for hiding this comment

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

are not affected by "dvc push/pull/status -c" - @efiop what would be the explicit way to explain this to someone less technical/DVC proficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shcheklein Not sure what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that this note should be probably written like this (if I understand its meaning correctly):

Note, the way to handle data described below can be considered an advanced alternative to an actual dvc add, dvc push workflow. In some cases you do need it indeed, in other cases it might be you have a better, simpler solution. If, for example, you are looking for a way to add data to DVC without moving it to the machine first, you might check this option straight-to-remote/cache, or dvc import-url ...

Otherwise my point that it's too low level, it will be very hard for folks exploring different ways of managing data with DVC to understand what implications does are not affected by dvc push/pull/status -c have, why is that a problem.

They way it is phrased now, it feels very abrupt be at the very beginning - no context, no explanation.

Copy link
Contributor Author

@efiop efiop Jan 24, 2021

Choose a reason for hiding this comment

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

The aim was to put a short and quick disclaimer to address the typical questions we get. I might need to get back to this later for a verbose explanation, and by that time straight-to-cache/remote might be out already and this will need to be even more verbose. Simple disclaimer is better than nothing, I would keep it for now.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think we are on the same page on this. I see all this questions and understand what you are trying to achieve. My only concern that they way it's written it won't move a needle since people won't understand this. People don't have enough context when they come here. For example, they might easily think that "advanced" is for them (why not, right?). If we don't know how to explain it better, I'm fine - let's merge it as is, may be it'll change it a bit indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is to save time for now.

@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-eu1luq January 24, 2021 19:49 Inactive
@efiop efiop merged commit e2dc69e into master Feb 1, 2021
jorgeorpinel added a commit that referenced this pull request Feb 4, 2021
@jorgeorpinel jorgeorpinel deleted the efiop-patch-1 branch February 7, 2021 03:06
shcheklein pushed a commit that referenced this pull request Mar 14, 2021
* guide: disclaim x data (impro #2104)

* guide: revert Exp Outs guide rename
per #2154 (review)
shcheklein pushed a commit that referenced this pull request Mar 29, 2021
) (#2214)

* guide: disclaim x data (impro #2104)

* Added changes from PR #2188 and modified paths & titles

- Changes title of "Data Access" to "Data and Model Access"
- Changes title of "Data Versioning" to "Data and Model Versioning"
- Renames path of Data Access and Data Versioning to
  `data-and-model-access.md` and `data-and-model-versioning.md`
  respectively.
- Adds redirects
-- `/doc/start/data-access` -> `/doc/start/data-and-model-access`
-- `/doc/start/data-versioning` ->
`/doc/start/data-and-model-versioning`
- Replaces links in `/doc/start` with the new links.

* Update redirects-list.json with fixed subsection redirects.

Co-authored-by: Jorge Orpinel <[email protected]>

* Fixed incomplete looking sentence

* merged into a single paragraph

* Divided models sentence and added "large files" phrase.

* Adds new paths to sidebar

* Updated links to data-access and data-versioning cmd ref

* updated links to data-access and data-versioning in blog

* Updated links to data-access and data-versioning in UC

* Updated links to data-access and data-versioning in UG

* updated yarn.lock

* Update content/docs/start/data-and-model-versioning.md

Co-authored-by: Jorge Orpinel <[email protected]>

* Restyled by prettier

* fixes hardcoded links to data-and-model-access in the blog

* minor fixes

* guide: revert Exp Outs guide rename
per #2154 (review)

* start: emphasize models are files (assumption)

* start: roll back unnecessary changes

unnecessary for #2214

Co-authored-by: Jorge Orpinel <[email protected]>
Co-authored-by: Jorge Orpinel <[email protected]>
Co-authored-by: Emre Sahin <iex@levinas>
Co-authored-by: Restyled.io <[email protected]>
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