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

repro cmdref updates #1861

Merged
merged 19 commits into from
Nov 3, 2020
Merged

repro cmdref updates #1861

merged 19 commits into from
Nov 3, 2020

Conversation

imhardikj
Copy link
Contributor

@imhardikj imhardikj commented Oct 13, 2020

Fixes #1632

  • Update targets description
  • example for using repro with a specific target stage
  • -R option desc

Partially addresses #1824

@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 13, 2020 19:03 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 14, 2020 18:27 Inactive
@imhardikj imhardikj marked this pull request as ready for review October 14, 2020 18:32
@jorgeorpinel jorgeorpinel had a problem deploying to dvc-landing-reprocmd-8au5lq1iw October 15, 2020 22:02 Failure
@jorgeorpinel jorgeorpinel had a problem deploying to dvc-landing-reprocmd-8au5lq1iw October 15, 2020 22:03 Failure
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.

Thanks @imhardikj. This needs some more work on the options, and to think through the examples. Please see discussions above.

@rogermparent
Copy link
Contributor

rogermparent commented Oct 15, 2020

I saw this failed deploy, and I believe it's been affected by the recent change to how the community page's schema works. I believe merging master will fix this, and I'm trying it out locally.

I can do the push with my merged master if that's alright, and we'll be able to see if it fixes the deploy.

@jorgeorpinel
Copy link
Contributor

Thank for the note Roger. Please merge master @imhardikj, thanks.

@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 17, 2020 20:09 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 18, 2020 14:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 18, 2020 17:59 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 23, 2020 19:00 Inactive
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.

Please see #1861 (comment) above.

@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 25, 2020 17:59 Inactive
Comment on lines 112 to 115
- `-R`, `--recursive` - expands arguments that `targets` accepts to determine
the stages to be reproduced, by searching each target directory and its
subdirectories. This option only has an effect if directory paths are given as
`targets`.
Copy link
Contributor

Choose a reason for hiding this comment

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

First, this is a confusing text. It should be explained in simpler terms, shorter sentences, etc. Also, it has grammar errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Secondly we're missing the whole point of #1632... Maybe you're confused by the term "expands". One thing is to expand a path (explore recursively) — that's not what the issue is about at all. Just forget about that term...

This is about the targets argument of dvc repro (and how -RP affect it). Again (from #1861 (comment)), "i.e. usually targets only accept "Stage or path to dvc.yaml or .dvc file to reproduce", right? It does not say that paths to directories are accepted. But these options (-RP) only work for directories, so what's going on?"

If you can answer that question, you'll be closer to understanding what's needed here.

@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 27, 2020 19:31 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 29, 2020 15:46 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 29, 2020 15:49 Inactive
Comment on lines -17 to +18
targets Stage or .dvc file to reproduce
targets Stage or path to dvc.yaml or .dvc file to reproduce. Using -R,
directories to search for stages can also be given.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. We will need to submit a matching change in the core dvc repo. Are you able to try that too @imhardikj ? Thanks

Copy link
Contributor Author

@imhardikj imhardikj Oct 30, 2020

Choose a reason for hiding this comment

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

I checked -p, it doesn't work with directories. So the current description is correct I think.

submit a matching change in the core dvc repo

Sure I'll do this. Currently this is displayed for targets: Stages to reproduce. 'dvc.yaml' by default.
This should be updated to exact same text as above right: Stage or path to dvc.yaml or .dvc file to reproduce. Using -R, directories to search for stages can also be given.
Also this (https://github.com/iterative/dvc/blob/master/dvc/command/repro.py) is the file that needs to updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 29, 2020 20:52 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 29, 2020 20:55 Inactive
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.

OK this is good so far. I guess changing the usage block was way easier than what we were trying to do initially... 🤦

Let's just check whether -p needs clarification too. Are directories accepted as targets when -p is used? Please test and if so, update that option to clarify @imhardikj

Thanks

@shcheklein shcheklein temporarily deployed to dvc-landing-reprocmd-8au5lq1iw October 30, 2020 12:42 Inactive
@jorgeorpinel
Copy link
Contributor

Woot woot 🎉

@jorgeorpinel jorgeorpinel merged commit bd928e3 into master Nov 3, 2020
@jorgeorpinel jorgeorpinel deleted the reprocmd branch November 9, 2020 04:02
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.

repro: improvements to targets arg and -R option
4 participants