repo: Add repro from given out names functionality #5273

Vonski · 2021-01-14T19:03:14Z

I'm not sure if I should have added special test for this functionality.

Should the --glob functionality be preserved? If so, should handling for it be added to collect_granular(...)?

Fixes #3875

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

@pared

pared

One comment,
In any case, test in test_repro veryfing that repro by output is possible would be nice.

pared · 2021-01-15T18:11:28Z

dvc/repo/reproduce.py

-                    accept_group=accept_group,
-                    glob=glob,
-                )
+            granular_stages = self.stage.collect_granular(


Hmm, if we are to change this method, we will effectively make glob stop working. We should not do that.
Ideally we should support glob for collect_granular.
But, we could also use collect_granular as a fallback in a situation when glob is not provided and collect returns no results. That way it will be easier but we will need to create issue for supporting glob in collect_granular and do it in the future anyway.

Second approach is inconsistent, because it would not let user to do repro by outs with glob option. I'll try to implement glob handling logic in collect_granular. One more question. There wasn't any tests fails connected to missing glob functionality. Should I prepare a test that validates if globbing is possible?

Ok, as far as I understand the only acceptable solution is to upgrade collect_granular to support globbing and accept_group. May I try to do it here?

@Vonski Sure, go ahead, if you stumble upon any problems, feel free to ping us.

Now collect_granular handles glob also.

Vonski · 2021-01-18T15:33:11Z

One comment,
In any case, test in test_repro veryfing that repro by output is possible would be nice.

I've added one, is this check correct for this functionality?

skshetry · 2021-03-04T09:13:09Z

dvc/repo/__init__.py

@@ -410,6 +413,9 @@ def func(out):
            if recursive and out.path_info.isin(path_info):
                return True

+            if glob:
+                return fnmatch.fnmatch(out.path_info.name, path)


name is a basename, we should be checking for the str(path_info) which should also make it work with ../data/** kind of globs.

I changed that and added new test to check how globbing with directories in path works.

skshetry · 2021-03-04T09:15:21Z

dvc/repo/stage.py

@@ -299,14 +303,16 @@ def collect_granular(
            return [StageInfo(stage) for stage in self.repo.stages]

        stages, file, _ = _collect_specific_target(
-            self, target, with_deps, recursive, accept_group
+            self, target, with_deps, recursive, accept_group, glob=glob


Now that repro will support output name as a target, I don't think we should glob for the stage names at all. Or, we could think of a different flags to glob stage names.

Doing this way, if the glob does match within the dvc.yaml, it will try not to collect from other stages. Globbing outputs should be global.

skshetry · 2021-03-04T09:29:50Z

cc'ing @dberenbaum @shcheklein

Right now, we have a --glob only for the stage names. I'd say that we change it to --glob-stages and use --glob for outputs-based glob. 🙂

pared · 2021-03-05T10:16:04Z

@Vonski seems like we have a consensus. We need another glob flag. As @skshetry mentioned --glob-stages should be for globbing in context of stages, and --glob should be for outputs. Would you be willing to introduce this?

Vonski · 2021-03-05T11:08:59Z

@Vonski seems like we have a consensus. We need another glob flag. As @skshetry mentioned --glob-stages should be for globbing in context of stages, and --glob should be for outputs. Would you be willing to introduce this?

@pared Yeah, sure. I assume that it should be introduced in this PR. Correct me if I'm wrong.

pared · 2021-03-05T14:34:47Z

@Vonski yes, the change shall be included here

skshetry · 2021-03-17T06:11:18Z

@Vonski, ping. What's the status of this?

Looking at this, it seems we are asking quite a big feature/change. If you feel the same, how about we start by supporting the repro from given output name (just the title of the PR)? We could introduce other changes in successive PRs. What do you think? 🙂

Vonski · 2021-03-17T06:39:42Z

@skshetry Honestly, I don't know. I couldn't find time to think about it yet. It looks like today I'll have some, so in 12 hours from now I should be able to answer the question, or maybe even to push some implemented solution. What do you say?

Vonski · 2021-03-17T17:52:38Z

@skshetry @pared I pushed a preview of my idea of mutually exclusive split of old glob flag, into new glob for outs paths, and glob_stages for stages names. Please check if the logic of reproduce is correct. Is this assumption about mutual exclusiveness acceptable?

As I understand, I'll also need to change some code for command repro to accept new --glob-stages flag. Is that correct? If so, I would appreciate if you could point out the files that should be modified to achieve that.

pared

@Vonski

Is this assumption about mutual exclusiveness acceptable

Yes, if there will be need to support that, we will continue with this change. For now is fine. We should probably check in the repro command and throw Exception mentioning that both cannot be provided.

As I understand, I'll also need to change some code for command repro to accept new --glob-stages flag.

Correct, here is what I think needs to be done:

- add --glob-stages in repro command - take a look at --glob to see how to do that
- parse --glob-stages in _repro_kwargs
- fix test for parsing command

Vonski · 2021-03-24T20:58:09Z

@pared I hope to find some time for it before the end of the week, fyi.

Vonski · 2021-03-28T14:58:04Z

@pared I added requested changes.

dvc/command/repro.py

dvc/repo/stage.py

jorgeorpinel · 2021-04-07T19:44:20Z

Right now, we have a --glob only for the stage names. I'd say that we change it to --glob-stages and use --glob for outputs-based glob.

Q. Couldn't changing the behavior of --glob be a breaking change for some users? (Better leave for 3.0?)

p.s. I do like that it's more consistent with add/pull/push --glob now though.

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

dvc/command/repro.py

pared · 2021-04-16T14:05:56Z

Ok, it seems to me that we are stretching this too much, @jorgeorpinel it seems that besides some suggestions it's ok, right? @Vonski feel free to accept the suggestions and let's merge this change, docs can be finished later.

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

pared · 2021-04-16T14:19:07Z

@jorgeorpinel

Couldn't changing the behavior of --glob be a breaking change for some users? (Better leave for 3.0?)

Yes, this is a breaking change, however, after it, we still would be able to retain "old" behavior with the new flag --glob-stages. I think that because of that we should proceed and involve this change in the current version.

@dberenbaum @shcheklein am I right here?

dberenbaum · 2021-04-16T15:17:50Z

@jorgeorpinel

Couldn't changing the behavior of --glob be a breaking change for some users? (Better leave for 3.0?)

Yes, this is a breaking change, however, after it, we still would be able to retain "old" behavior with the new flag --glob-stages. I think that because of that we should proceed and involve this change in the current version.

@dberenbaum @shcheklein am I right here?

Hm, if someone is scripting their use of dvc repro --glob (like in cml), then this seems like it's going to break their workflow, so I'm not sure it's a good idea to introduce without sufficient warning. Maybe it would be better to go with --glob/--glob-outs instead of --glob/--glob-stages?

By the way, forgetting about glob, what happens if there is a stage and an output in a different stage with the same name?

pared · 2021-04-19T08:44:41Z

@dberenbaum

By the way, forgetting about glob, what happens if there is a stage and an output in a different stage with the same name?

In that case, both stages shall be reproduced.

pared · 2021-04-19T13:49:06Z

@dberenbaum

I'm not sure it's a good idea to introduce without sufficient warning. Maybe it would be better to go with --glob/--glob-outs instead of --glob/--glob-stages?

It is possible that that we might break some users scripts. I see 2 solutions to this problem:

accept the fact that something might fail
use --glob-outs/--glob approach.

While I am not a fan of breaking the consistency between minor dvc versions, it seems to me that in this case it would be justified by the fact (noted by @jorgeorpinel) that add/pull/push commands target outputs when using --glob.

So if we go with --glob-outs that puts us in situation where one needs to use different option when using add/pull/push (--glob) and other one (--glob-outs) when using repro command. From UI point of view it would seem to me to be tiresome to remember that fact.

dberenbaum · 2021-04-19T14:46:10Z

Before 1.0, users always specified outputs as targets to repro, right?

By the way, forgetting about glob, what happens if there is a stage and an output in a different stage with the same name?

In that case, both stages shall be reproduced.

Hmm, so do we recommend users avoid this? How would someone repro one or the other? Is this the same behavior for push/pull?

While I am not a fan of breaking the consistency between minor dvc versions, it seems to me that in this case it would be justified by the fact (noted by @jorgeorpinel) that add/pull/push commands target outputs when using --glob.

Makes sense, but those commands are inherently about files and repro is inherently about stages. Do push/pull glob stages?

pared · 2021-04-20T09:32:30Z

Hmm, so do we recommend users avoid this? How would someone repro one or the other? Is this the same behavior for push/pull?

We don't do that in any way - glob option is there to switch this on or off. If one wants to reproduce only particular output, dvc repro should be called for particular stage. Using glob is a convenience method allowing user to target multiple stages containing assets matching the target. In case of push/ pull its similar - one should not use glob if aiming to interact with some particular target. If users would still like to use glob to repro only particular output, full path needs to be provided.

Makes sense, but those commands are inherently about files and repro is inherently about stages. Do push/pull glob stages?

No, it does not.

dberenbaum · 2021-04-20T12:58:54Z

Thanks @pared, makes sense!

I'm still unsure about introducing a breaking change in a minor release like this. Would be interested in getting thoughts from others @jorgeorpinel @efiop. Is changing from --glob to glob for stages to --glob/--glob-stages to glob for files/stages enough of a breaking change to warrant holding off until a major release?

efiop · 2021-04-21T13:46:54Z

Btw, --glob is like that right now because we want to turn it on by default once we are ready. And we won't be able to turn both --glob and --glob-stages on without mixing them together. And that's actually probably what we really want here in the long term: to make --glob work for both stages and outputs and even dvc-files - that would be the best ui, really. So right now it would be nice to merge --glob and --glob-stages into one --glob, if that makes sense.

@skshetry considering the implications that we already have in collect logic for outputs/stages/granular outputs, what do you think about ^^^ ?

jorgeorpinel · 2021-04-21T17:39:00Z

better to go with --glob/--glob-outs ? ... add/pull/push commands target outputs when using --glob.

I'd also err on the side of not breaking anyone's scripts for now.

Also keep in mind repro may be phased out into 3.0? In favor of exp run. Will this new targeting (+ globing) make sense for exp run? (Do you "run" a data artifact)?

if there is a stage and an output (in a different stage) with the same name ... In that case, both stages shall be reproduced

Would be ideal to error-out instead with an appropriate message, and a way to specify whether you meant the stage or the output. But prob not critical so could be left for a follow-up PR. And again will this all pass on from repro to exp run? 🙂

would be nice to merge --glob and --glob-stages into one --glob

Agree. Another route to consider: leave --glob changes out of this PR if possible (wouldn't support file/dir paths for now in repro).

skshetry · 2021-04-22T11:50:46Z

And we won't be able to turn both --glob and --glob-stages on without mixing them together.

It will be kinda confusing mixing outputs and stages both for glob. I have been against the change for supporting output names as a target in repro, but with glob too in the mix, it adds more confusion.

Internally, it complicates things as well, as it's already quite hard to support multiple targets, optimizing for stages names and directories and provide good error message as well. So I'd prefer avoiding it as much as possible.

even dvc-files

Why would we need dvc-files here?

@dberenbaum, @jorgeorpinel, @efiop, as mentioned by @pared, I don't think the glob discussion is worth it for blocking this PR, especially considering the PR is sitting there for more than 3 months now. Let's discuss this in a separate issue.

efiop · 2021-04-22T23:58:24Z

Thanks for a great discussion, guys! 🙏

We had a great discussion with @skshetry about this today and so, to summarize: we can't merge this as is, as it is breaking backward compatibility and creates a problem for us in the near future when we will enable --glob by default. The latter problem applies to alternative --glob-outs approach, so it is also a nogo. That being said, I see that @skshetry has drawn a pretty nice plan in #5273 (comment) , where he raised a concern that this PR is doing too much, and I tend to agree with him. In order to support outs globbing we need to first support using outputs in a separate PR (along with figuring out a way to distinguish output name from a stage name in a deterministic way, which is not trivial) and then we can come back to the globbing itself that should be part of regular --glob (there are lots of questions about distinguishing stages/outputs there too, so it will be quite a substantial effort to get it right).

@Vonski Thank you so much for your contributions! 🙏 I hope you are not feeling discouraged by this, it is just a classic issue that turned out to be much more complex than originally anticipated, and all of these discussions and research are arguably an even bigger contribution to the project than the PR itself, so we truly appreciate it.

So there are three paths we could take from here:

close this PR and start from scratch, tackling output support first, merging it and then proceeding with globbing
repurpose this PR for only tackling outputs (without globbing) and only then proceeding with globbing in a separate PR
considering that the original issue doesn't seem to be in high demand (it is a bit surprising, but still), we might just decide that less is more and not implement it for now, until there is a clear demand from users

@Vonski What do think?

efiop · 2021-04-26T11:11:16Z

Closing for now.

Vonski · 2021-04-26T18:50:29Z

Ah sorry I'm late. I would appreciate to start from scratch and try to push issue forward anyway. I think 1. option is the best idea under the circumstances. I am also a little more engaged in some other things right now, so I have less time to make things happen here, but if it is not a problem, I want to help with that. I hope I'll find some time this week to create another PR for this.

efiop · 2021-04-26T18:57:37Z

@Vonski Sounds good! Thank you! 🙏

pared assigned pared and unassigned pared Jan 15, 2021

pared self-requested a review January 15, 2021 14:08

pared suggested changes Jan 15, 2021

View reviewed changes

efiop added the awaiting response we are waiting for your reply, please respond! :) label Jan 24, 2021

efiop changed the title ~~repo: Add repro from given out names functionality~~ [WIP] repo: Add repro from given out names functionality Jan 24, 2021

Vonski changed the title ~~[WIP] repo: Add repro from given out names functionality~~ repo: Add repro from given out names functionality Feb 11, 2021

pared added enhancement Enhances DVC ui user interface / interaction and removed awaiting response we are waiting for your reply, please respond! :) labels Feb 11, 2021

pared self-requested a review February 14, 2021 08:21

pared approved these changes Feb 14, 2021

View reviewed changes

pared requested review from a team and removed request for a team February 14, 2021 08:21

efiop requested a review from skshetry March 3, 2021 13:28

skshetry reviewed Mar 4, 2021

View reviewed changes

pared reviewed Mar 21, 2021

View reviewed changes

repo: Change help string for targets in dvc repro

9ade4f8

jorgeorpinel reviewed Apr 7, 2021

View reviewed changes

dvc/command/repro.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Apr 7, 2021

View reviewed changes

dvc/command/repro.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Apr 7, 2021

View reviewed changes

dvc/repo/stage.py Outdated Show resolved Hide resolved

Update dvc/repo/stage.py

a187ad4

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

jorgeorpinel mentioned this pull request Apr 8, 2021

remove: output file/dir by path iterative/dvc.org#2357

Closed

Vonski and others added 3 commits April 9, 2021 15:32

Update dvc/command/repro.py

5f51be7

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Update dvc/command/repro.py

9112f24

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Fix formatting of help msg

07a4d73

jorgeorpinel reviewed Apr 13, 2021

View reviewed changes

dvc/command/repro.py Outdated Show resolved Hide resolved

Update dvc/command/repro.py

b6be156

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

skshetry mentioned this pull request Apr 22, 2021

dvc pull --glob tries to pull dvcignored files and glob pattern is only applied to files that already exist #5864

Open

efiop added the awaiting response we are waiting for your reply, please respond! :) label Apr 23, 2021

efiop closed this Apr 26, 2021

repo: Add repro from given out names functionality #5273

repo: Add repro from given out names functionality #5273

Conversation

Vonski commented Jan 14, 2021 • edited Loading

pared left a comment

Choose a reason for hiding this comment

pared Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Vonski Jan 18, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

Vonski Jan 18, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

pared Jan 19, 2021

Choose a reason for hiding this comment

Vonski Feb 11, 2021

Choose a reason for hiding this comment

Vonski commented Jan 18, 2021

skshetry Mar 4, 2021

Choose a reason for hiding this comment

Vonski Mar 17, 2021

Choose a reason for hiding this comment

skshetry Mar 4, 2021

Choose a reason for hiding this comment

skshetry Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

skshetry commented Mar 4, 2021

pared commented Mar 5, 2021

Vonski commented Mar 5, 2021

pared commented Mar 5, 2021

skshetry commented Mar 17, 2021

Vonski commented Mar 17, 2021

Vonski commented Mar 17, 2021

pared left a comment • edited Loading

Choose a reason for hiding this comment

Vonski commented Mar 24, 2021

Vonski commented Mar 28, 2021

jorgeorpinel commented Apr 7, 2021 • edited Loading

pared commented Apr 16, 2021

pared commented Apr 16, 2021

dberenbaum commented Apr 16, 2021

pared commented Apr 19, 2021

pared commented Apr 19, 2021

dberenbaum commented Apr 19, 2021

pared commented Apr 20, 2021

dberenbaum commented Apr 20, 2021

efiop commented Apr 21, 2021

jorgeorpinel commented Apr 21, 2021 • edited Loading

skshetry commented Apr 22, 2021

efiop commented Apr 22, 2021

efiop commented Apr 26, 2021

Vonski commented Apr 26, 2021

efiop commented Apr 26, 2021

Vonski commented Jan 14, 2021 •

edited

Loading

pared Jan 15, 2021 •

edited

Loading

skshetry Mar 4, 2021 •

edited

Loading

pared left a comment •

edited

Loading

jorgeorpinel commented Apr 7, 2021 •

edited

Loading

jorgeorpinel commented Apr 21, 2021 •

edited

Loading