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 config.md for #6419 (i.e. #6111) #2776

Merged
merged 11 commits into from
Oct 5, 2021

Conversation

meierale
Copy link
Contributor

@meierale meierale commented Sep 1, 2021

Updated the config.md to document the new state.dir and index.dir options.

Per iterative/dvc#6419

Updated the config.md to document the new state.dir and index.dir options.
Syntax sugar, trying to satisfy the checks.
@jorgeorpinel jorgeorpinel added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Sep 7, 2021
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.

A few initial copy edits

content/docs/command-reference/config.md Outdated Show resolved Hide resolved
content/docs/command-reference/config.md Outdated Show resolved Hide resolved
content/docs/command-reference/config.md Outdated Show resolved Hide resolved
Comment on lines 244 to 246
(By default, store SQLite-based remote indexes and state's
`links` and `md5s` caches in the repository itself to avoid any
possible state corruption in 'shared cache dir' scenario.)

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is about default behavior, I thin kit would be best to document that in https://dvc.org/doc/user-guide/project-structure/internal-files instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW https://dvc.org/doc/user-guide/project-structure/internal-files doesn't mention "state file" so probably a terminology update is needed in this doc or in that other one? I.e. what internal files does state.dir affect exactly? (I guess .dvc/tmp/md5s and .dvc/tmp/links) Thanks

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@jorgeorpinel
Copy link
Contributor

So iterative/dvc#6419 is still undecided though? We may want to wait until that's near final to finish this PR. Please remind me when to look again @meierale ! Thanks

meierale pushed a commit to meierale/dvc.org that referenced this pull request Sep 13, 2021
As suggested by @jorgeorpinel, I moved the default behavior description of `md5s` and `links`dirs to here. (from iterative#2776)
@meierale
Copy link
Contributor Author

So iterative/dvc#6419 is still undecided though? We may want to wait until that's near final to finish this PR. Please remind me when to look again @meierale ! Thanks

Hi @jorgeorpinel - I suggest to prepare the docs (this PR and #2826) so that they are ready when needed. But only merge when #6419 is being merged as well.
@efiop: Now that 2.7.x are out, are you still going to merge #6419 and introduce your changes on top? Or what's the current plan/approach?

@efiop
Copy link
Contributor

efiop commented Sep 13, 2021

Now that 2.7.x are out, are you still going to merge #6419 and introduce your changes on top? Or what's the current plan/approach?

@meierale Correct, waiting for the right moment with other stuff getting in and needing a release.

@jorgeorpinel
Copy link
Contributor

I suggest to prepare the docs (this PR and #2826)

Absolutely, thanks. I just wonder whether iterative/dvc#6419 can change in a way that requires reviewing the docs further so please lmk once that is approved to make a final review. I'll probably commit some suggestions for now though.

Comment on lines 242 to 244
- `state.dir` - specify a custom location for the state file. This may be
necessary when using DVC on NFS or other mounted volumes.
⚠️ Make sure no other project shares the same `state.dir` location.
Copy link
Contributor

@jorgeorpinel jorgeorpinel Sep 13, 2021

Choose a reason for hiding this comment

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

Sorry, still confused. Which state file does this refer to? https://dvc.org/doc/user-guide/project-structure/internal-files doesn't use that term ("the state file") and the note introduced in #2826 doesn't clarify this either.

Does this change the location of .dvc/tmp/md5s or .dvc/tmp/links ? Or of both

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 tried to document it more clearly: state.dir affects the links and md5s directories, index.dir the index directory.

describe more in detail which folders are affected with the state.dir config entry.
@efiop
Copy link
Contributor

efiop commented Sep 18, 2021

Thanks for the PR, @meierale ! 🙏

@jorgeorpinel Let's hold on this PR for now, please, we need to reconsider some stuff on dvc side and we'll then come back to this ourselves.

@jorgeorpinel
Copy link
Contributor

OK @efiop closing for now then... But iterative/dvc#6419 was merged so shouldn't we have this documented? And yes thanks again @meierale !

@efiop
Copy link
Contributor

efiop commented Oct 2, 2021

Ok, keeping and releasing this for now. So reopening.

@efiop efiop reopened this Oct 2, 2021
@jorgeorpinel
Copy link
Contributor

Merged this via #2891 but this is still showing open for some reason... 🤔

The changes are already in master: https://github.com/iterative/dvc.org/blob/master/content/docs/command-reference/config.md#index

@jorgeorpinel
Copy link
Contributor

Resolved conflicts, now this PR has 0 changes 🤷 Merging!

@jorgeorpinel jorgeorpinel merged commit 69f6e83 into iterative:master Oct 5, 2021
@jorgeorpinel
Copy link
Contributor

Also finish Add note regarding default behavior of sqlite dirs #2826

Need to do that though...

@jorgeorpinel
Copy link
Contributor

p.s. thanks for taking over @efiop

jorgeorpinel added a commit that referenced this pull request Oct 5, 2021
* Add note regarding default behavior of sqlite dirs

As suggested by @jorgeorpinel, I moved the default behavior description of `md5s` and `links`dirs to here. (from #2776)

* Update internal-files.md

adjust documentation according to how it works in the PR ;-)9

* config.index: yarm format

* config: corrections on index.dir and state.dir

Co-authored-by: Jorge Orpinel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌛ status: wait-core-merge Waiting for related product PR merge/release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants