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

Misc. updates #1615

Merged
merged 56 commits into from
Aug 5, 2020
Merged

Misc. updates #1615

merged 56 commits into from
Aug 5, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jul 22, 2020

Triggered as out-of-scope from other tickets/PRs

@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 22, 2020 19:48 Inactive
@jorgeorpinel jorgeorpinel mentioned this pull request Jul 22, 2020
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 22, 2020 21:54 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 23, 2020 19:08 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 23, 2020 19:19 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 23, 2020 19:36 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 23, 2020 19:39 Inactive
@sarthakforwet

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 23, 2020 22:08 Inactive
@jorgeorpinel

This comment has been minimized.

@iterative iterative deleted a comment from sarthakforwet Jul 23, 2020
@jorgeorpinel jorgeorpinel marked this pull request as ready for review July 23, 2020 22:29
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-pjpofhyw506r July 24, 2020 16:06 Inactive
@iterative iterative deleted a comment from sarthakforwet Jul 24, 2020

- Not enough isolation/granularity - commands like `dvc metrics diff`, `dvc dag`
and others by default dump all the metrics, all the pipelines, etc.
- By default, DVC commands like `dvc checkout` and `dvc repro` explore the whole
Copy link
Member

Choose a reason for hiding this comment

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

still, case with not isolated/granular dvc metrics diff seems a bit different than dvc pull - one is mostly about performance, second is about actual output - might resonate with different people.

It's fine though to keep it this way. Not a strong one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I changed the last sentence to "This can produce undesirable results and/or be inefficient for large monorepos." to better capture both cases.

Copy link
Member

Choose a reason for hiding this comment

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

yep, but this is a good example of what I meant with implicit vs explicit. undesirable results sounds and reads cool, and one paragraph looks better for sure, but it almost does not mean anything to a reader .. it's hard (or at least takes an effort) to get an example of what does it mean.

again, not a big deal in this case, just want to further explain my thought-process on being explicit if possible.

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 see. OK, split into 2 bullets for clarity in be5bc6d.


```dvc
$ cd project-A
$ dvc pull
project-A # full DVC + Git repo
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this comment?

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 they are informative. Changed to git init && dvc init in cac92e5. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

so, that's probably what I don't understand - if we put them they highlight importance of something to my mind, I spend a cycle thinking why did author put a comment here - is .git important in this case? does it change anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do intent to highlight exactly what we mean by nested or not-nested --subdir projects:

Not nested: single-level DVC projects in a Git repo.
Nested: multi-level DVC projects inside (DVC projects inside) DVC/Git repo

This could be also just undetstood by looking at the directories shown (.git, .dvc) but it's not as obvious).

Copy link
Member

Choose a reason for hiding this comment

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

.git still not important here to my mind unless I'm missing something. Nesting would work with --no-scm the same way effectively.

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'm removing these block examples completely for now, and extracting to a following PR...

that even if the project is tracked by Git, or if Git is initialized in it
later, DVC will keep operating detached from Git in this project.

> Note that like with nested <abbr>repositories</abbr> and `--subdir` projects,
Copy link
Member

Choose a reason for hiding this comment

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

so, if behavior is the same, do we actually need this note?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Aug 5, 2020

Choose a reason for hiding this comment

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

Ieally not but yes in the way things are structured now in this ref. This isolation and nested situation got too complicated... I should probably make a separate sub-section to cover it after explaining all the 3 kinds of projects (typical or git-enabled, --subdir, --no-scm).

Can we extract to another issue/PR and get this merged maybe? I have some other changes I also want to throw in on another regular updates PR 😬

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it then, create a ticket and discuss the changes in advance?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Aug 5, 2020

Choose a reason for hiding this comment

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

Removed in 9c3ba77. I have all the text already in another branch though so I'll just open a draft PR to discuss, if you don't mind... ⌛

@jorgeorpinel
Copy link
Contributor Author

OMG sorry I linked the other PR to this one 😋 Reopening...

@jorgeorpinel jorgeorpinel reopened this Aug 5, 2020
@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 04:57 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 05:03 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 05:04 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 05:09 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 05:15 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-icpfouh5axbm August 5, 2020 19:13 Inactive
@jorgeorpinel jorgeorpinel requested a review from shcheklein August 5, 2020 19:13
@shcheklein
Copy link
Member

I've approved it already a bit ago. Merging ...

@shcheklein shcheklein merged commit 211c0f9 into master Aug 5, 2020
@jorgeorpinel
Copy link
Contributor Author

Thanks

shcheklein pushed a commit that referenced this pull request Aug 5, 2020
* cmd: review repro examples
per #1572 (comment)

* cmd: fic tyupo in get-url

* cmd: updates to repro
per #1572 (review)

* cmd: rewrite repro -P desc
rel #1572 (review)

* cmd: simplified and generalize repro targets desc and DVC file mention
per #1572 (comment)
and #1572 (comment)

* cmd: minor update for repro desc wording
per #1572 (comment)

* term: don't use "synchronize" in the context of checkout

* cmd: rewrite Downstream example and added info for sequential execution of stages

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* cmd: Updated Downstream example

* Update content/docs/command-reference/repro.md

* repro: Updated Downstream example

* Update content/docs/command-reference/repro.md

* cmd: updated last para for the description of --downstream and improved formatting

* cmd: review language of init --subdir

* term: revuew usage of "granular", esp. around init --subdir

* repro.md: updated Downstream example

* cmd: improve init --subdir explanation

* cmd: add info about nested subrepos to init

* cmd: fix -P option desc.
per #1615 (review)

* cmd: improve explanation on how --subdir affects commands
per #1615 (review)

* cmd: simplify nested structures explanation in init
per #1615 (comment)

* guide: add note aboud `cp` not being a download in external deps
per #1643 (review)

* cmd: add note about what --cwd means to repro
per iterative/dvc#4292 (comment)

* guide: nvmd! removing that note in external deps
per 42b670f#r41087633

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* cmd: more small updates to init

* Update content/docs/command-reference/repro.md

* Update content/docs/command-reference/repro.md

* Restyled by prettier

* cmd: rewrap metrics diff usage paragraph

* term: remove "just" from -j desc in 3 refs
per eda27fc

* cmd: add command examples to init --subdir use cases
per #1615

* cmd: explain nested repo and projects of all kinds outside of --subdir
per #1615 (review)

* cmd: remove bold names to nested and not-nested structure examples in init --subdir
per #1615 (review)

* cmd: standardize --jobs option in all refs
per #1615 (review)
et al.

* cmd: add speed note to --jobs desc in all refs.
per #1615 (review)

* cmd: change versioning command example in init
per #1615 (review)

* cd: change repo comments in init --subdir examples
for #1615 (review)

* cmd: improve note on DVC submodules a little
for #1615 (review)

* cmd: better explain why isolation is important in --subdir bullet
per #1615 (review)

* cmd: split last --subdir cases explicitly as 2 bullets
per #1615 (comment)

* cmd: remove most notes and code block examples about nesting projects/repos in init
per #1615 (review)

* cmd: add basic text about nesting to init

* cmd: revert nested block examples back into --subdir
per #1661 (review)

* cmd: remove new section about nesting to revert more

Co-authored-by: sarthakforwet <[email protected]>
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.

cmd: explain init dvc nested dvc projects/repos and relationship to --subdir
5 participants