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

add repro --recursive in Repro command reference #396

Closed
wants to merge 13 commits into from
Closed

add repro --recursive in Repro command reference #396

wants to merge 13 commits into from

Conversation

kurianbenoy
Copy link
Contributor

@kurianbenoy kurianbenoy commented Jun 2, 2019

Fix #367

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 great, a few things to fix. Thanks! 🎉

@kurianbenoy
Copy link
Contributor Author

@shcheklein why is the build getting failed?

@shcheklein
Copy link
Member

@kurianbenoy disregard it. We've just enabled it and we need to tune CirlcCI configs to do something meaningful.

subdirectories for DVC-files to download data for. Along with providing a
`target`, or `target` along with `--with-deps` it is yet another way to cut
the scope of DVC-files to download.
- `-R`, `--recursive` - `targets` values is expected to be a directory path
Copy link
Member

Choose a reason for hiding this comment

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

very hard to understand what's going on here. The primary intention of -R is to accept directories and search for the DVC-files inside them. It accepts files along with directories for simplicity. For directories it goes down recursively, for files it behaves in a regular way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the message now ok @shcheklein?

I will make the subsequent changes tomorrow

Copy link
Member

Choose a reason for hiding this comment

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

No, not really :( Can you try to explain me in this thread what this option is about? In a few simple sentences. Like 1, 2, 3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For dvc repro -R

  1. Targets can be a file or directory
  2. What it does is reproduce all stages for specified target
  3. --with-deps cuts scopes of DVC-ffiles being reproduced or staged

When I tried locally, there was no difference in dvc repro and dvc repro -R for a specific target. I don't correctly understand the use case of -R flag. Can you please explain @shcheklein

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.

  1. Hard to understand the explanation
  2. git grep '-R' shows that fetch at least should be changed. Ask on Discord about commit, checkout, and metrics show.

@@ -6,7 +6,7 @@ Remove unused objects from cache or remote storage.

```usage
usage: dvc gc [-h] [-q | -v] [-a] [-T] [-c]
[-r REMOTE] [-f] [-j JOBS]
[-r REMOTE] [-f] [-j JOBS] [- R]
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jun 7, 2019

Choose a reason for hiding this comment

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

There's a space between - and R.
But anyway, the usage shown with dvc gc -h (latest DVC version: 0.41.3+1e7e2a) is

usage: dvc gc [-h] [-q | -v] [-a] [-T] [-c] [-r REMOTE] [-f] [-j JOBS]
              [-p [REPOS [REPOS ...]]]

Which doesn't include -R (and doesn't match the current docs 😓 )

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 and it dvc -R is not mentioned in docs for a no of other commands as well like fetch, status, metrics,gc and somemore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. So did you update this usage and remove the -R option from this doc?

Could you please open an issue for the docs that are missing the -R option and need update in their usage 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.

Opened issue iterative/dvc#2154_

Copy link
Member

Choose a reason for hiding this comment

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

there still a space in this file between - and R

Copy link
Contributor

Choose a reason for hiding this comment

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

@kurianbenoy Where did you get gc -R from anyways?

@kurianbenoy kurianbenoy mentioned this pull request Jun 8, 2019
2 tasks
or directory, relative to current location(eg.`data/models`). Determines
DVC-files to download data for the target directory. Along with providing a
`target`, and/or `--with-deps` it is yet another way to cut the scope of
DVC-files to download.
Copy link
Member

Choose a reason for hiding this comment

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

@kurianbenoy it's better. can you explain me what does this option mean in your own words, please? :) It's still hard to read, I still don't understand from this description what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For dvc pull -R

  1. Targets can be a file or directory
  2. What it does is pulls cache for new DVC-files from the subdirectories inside DVC remote storage
  3. --with-deps cuts scopes of files being pulled

Copy link
Member

Choose a reason for hiding this comment

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

All true, but emphasizing it properly is important.

  1. It was made specifically for a case when target is a directory (to search inside them for the DVC-files). It accepts files just for simplicity. So, that you don't have to run a separate command if you have a mix of both. The way we describe it should match to the primary case - when people are looking for a recursive search inside a dir.
  2. It pulls data from the cache to the workspace (and downloads them from remote first if needed, in a way it is described in this document) for data files and directories that are referenced in those DVC-files. I'm not sure I understand the new part in your sentence.
  3. Along with providing a target, and/or --with-deps it is yet another way to cut the scope of DVC-files to download. - let's just remove it completely, I don't see any value in it to be honest.

named directory, and its subdirectories, for DVC-files for which to commit
data. Along with providing a `target`, or `target` along with `--with-deps`,
it is yet another way to limit the scope of DVC-files to upload.
- `-R`, `--recursive` - determines the files to commit cache- by searching the
Copy link
Member

Choose a reason for hiding this comment

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

commit cache-?

This comment was marked as off-topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait... Me again, SORRY.

Actually I DID update this option already. (I had forgotten to push the commit to my PR until now). See for example https://github.com/iterative/dvc.org/pull/441/files#diff-cf1201ba4e234c56d863d0adad07fc7aR82

Copy link
Member

Choose a reason for hiding this comment

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

@jorgeorpinel have you also added -R to repro? That was the original scope of this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like you have not added -R option in dvc commit

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jun 25, 2019

Choose a reason for hiding this comment

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

I had not until now. Added the change o my newest PR now (See #[email protected])

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.

Needs more work still. I've sent a comment in private and left a few comments here.

@shcheklein
Copy link
Member

@kurianbenoy closing it due to inactivity. Let me know if you decide to give it another try, and check @jorgeorpinel latest changes. You might want to just take them as a base for repro -R

@shcheklein shcheklein closed this Jun 24, 2019
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.

add repro -R to docs, fix -R description in other commands
4 participants