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

ref: repro accepts outputs as targets #2362

Closed
wants to merge 10 commits into from
Closed

Conversation

Vonski
Copy link

@Vonski Vonski commented Apr 7, 2021

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. 🙏

@Vonski
Copy link
Author

Vonski commented Apr 7, 2021

Related iterative/dvc#5273

@jorgeorpinel
Copy link
Contributor

We should also update the part of the Description that reads "There are a few ways to restrict what will be regenerated by this command..."


- `-R`, `--recursive` - looks for `dvc.yaml` files to reproduce in any
directories given as `targets`, and in their subdirectories. If there are no
directories among the targets, this option has no effect.

- `--glob` - causes the `targets` to be interpreted as wildcard
[patterns](https://docs.python.org/3/library/glob.html) to match for output
names. For example: `output*` (certain output names). Note that it
Copy link
Contributor

@jorgeorpinel jorgeorpinel Apr 7, 2021

Choose a reason for hiding this comment

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

Suggested change
names. For example: `output*` (certain output names). Note that it
files or directories. For example: `output*` (certain outputs). Note that it

@jorgeorpinel
Copy link
Contributor

@Vonski thanks for this! Please try to fix the md formatting if possible (after applying my suggestions). See https://dvc.org/doc/user-guide/contributing/docs 🙂

@jorgeorpinel jorgeorpinel added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Apr 7, 2021
@Vonski
Copy link
Author

Vonski commented Apr 8, 2021

@jorgeorpinel I'll wait with updating things here until we agree on what terms should be used in help strings in core PR to make docs consistent with them.

@jorgeorpinel
Copy link
Contributor

Sorry for the delay @Vonski, getting back to this now. I see the core PR is still unmerged so I guess we still have some time to iterate, but we can also take this over when needed so no pressure (please lmk if your capacity is limited thouogh). Thanks! 👇

Comment on lines 49 to 53
There are a few ways to restrict what will be regenerated by this command: by
specifying specific reproduction [`targets`](#options), or by using certain
command [options](#options), such as `--single-item` or `--all-pipelines`.
command [options](#options), such as `--single-item` or `--all-pipelines`. It is
also possible to use `--glob` and `--glob-stages` to match targets with
[glob](<https://en.wikipedia.org/wiki/Glob_(programming)>) patterns.
Copy link
Contributor

@jorgeorpinel jorgeorpinel Apr 13, 2021

Choose a reason for hiding this comment

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

In #2362 (comment) I meant: we should mention that another way is to specify stage outputs (which will cause the whole stage to be reproed (right?)).

No need to change general info. about --glob in this PR (only further down to update it as per this PR's actual topic: stage outputs as targets.)

Copy link
Author

@Vonski Vonski Apr 16, 2021

Choose a reason for hiding this comment

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

I need a bit of clarification here. If we don't want information about *glob* flags here (thank you for pointing it out) I don't see what should be added to this paragraph, because I understand that from now on generic target could be also any defined stage output (and it is described in section about targets, to which target word links). So imo this part should be left as it was at the beginning (without any changes that I've added). Are we on the same page about that? @jorgeorpinel

Copy link
Contributor

Choose a reason for hiding this comment

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

We were on the same page, yes.

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.

Suggestions for the Options section:

content/docs/command-reference/repro.md Outdated Show resolved Hide resolved
- `dvc repro modeling/dvc.yaml:prepare`: Stage in a specific `dvc.yaml` file
- `dvc repro --glob train-*`: Pattern to match groups of stages
- `dvc repro --glob 'model.??'`: Pattern to match groups of output filenames
- `dvc repro --glob-stages 'train-mode?'`: Pattern to match groups of stages
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `dvc repro --glob-stages 'train-mode?'`: Pattern to match groups of stages
- `dvc repro --glob-stages cleaning/dvc.yaml:parse*`: Pattern to match groups
of stages (by path and name)

Copy link
Author

@Vonski Vonski Apr 16, 2021

Choose a reason for hiding this comment

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

Question here: Is it intended approach to show commands with globs without putting them inside apostrophes (globs I mean). I don't know how does it work in other shells, but on my machine it will scan for filenames that match before arguments are passed to dvc repro command. Should it be like that? @jorgeorpinel

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it would need 's to avoid the shell's built-in globbing.

content/docs/command-reference/repro.md Outdated Show resolved Hide resolved
content/docs/command-reference/repro.md Outdated Show resolved Hide resolved
@Vonski
Copy link
Author

Vonski commented Apr 13, 2021

No problem @jorgeorpinel! I'm cool with fixing this to the very end. I won't have time until Thursday though. If it can wait I am happy to do that.

@jorgeorpinel
Copy link
Contributor

Sure. The core PR isn't even merged yet 🙂 No rush

Vonski and others added 4 commits April 16, 2021 16:33
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@jorgeorpinel jorgeorpinel changed the title dvc repro: Add info about possibility of usage output names as targets ref: repro accepts outputs as targets Apr 20, 2021
@shcheklein
Copy link
Member

Closing this since the DVC one was closed ? Sorry about that @Vonski , and we really appreciate the effort here.

@shcheklein shcheklein closed this Apr 28, 2021
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