add option to require explicit fetch/checkout #10451

pbailey-hf · 2024-06-06T20:19:44Z

This PR adds the explicit field to outs in .dvc files. outs marked as explicit must be explicitly mentioned on the command line to be affected by fetch, checkout, or pull.

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

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

dberenbaum · 2024-06-21T20:27:08Z

dvc/repo/fetch.py

@@ -55,6 +55,8 @@ def stage_filter(stage: "Stage") -> bool:
    def outs_filter(out: "Output") -> bool:
        if push and not out.can_push:
            return False
+        if out.explicit and not any(targets):


Should out be compared to each target to see if it matches any of the targets? It looks like this would fetch out anytime any target is specified, even if the target does not match the explicit out.

I originally went down that path, then it occurred to me that the usual target matching logic would further filter the list that this function returns, so there should be no need to reimplement that logic. I updated the unit test to demonstrate and build confidence.

dberenbaum

LGTM, thanks! @skshetry Could you take a look please?

shcheklein · 2024-06-24T22:03:46Z

one ask (if we decide to go with this) - can we find a better name please? explicit can mean anything (explicit output), explicit path? explicit remote?, etc, etc

if this options is about pull / push / fetch it should have a name ideally related to that. From the top of my head:

pull: auto | explicit | None

if it affects both push and pull (does it?):

sync: ...

etc. Let's brainstorm here a bit please.

thanks @pbailey-hf !

pbailey-hf · 2024-06-25T19:11:23Z

if this options is about pull / push / fetch it should have a name ideally related to that. From the top of my head:

It only affects fetch/checkout and by extension pull. I like your suggestion of calling it

pull: auto|explicit|None (=default auto)

dberenbaum · 2024-06-25T19:52:20Z

Agreed, but I would suggest slightly different naming to better match the existing push options in https://dvc.org/doc/user-guide/project-structure/dvc-files#output-entries:

pull: true|false|explicit (=default true)

Edit: note that this also implies that we add pull: false, which is not implemented now, but should be straightforward to do similarly to explicit.

pbailey-hf · 2024-06-25T19:54:58Z

That makes sense to me. I will implement it shortly.

pbailey-hf · 2024-06-25T20:35:14Z

dvc/output.py

@@ -1501,6 +1507,7 @@ def _merge_dir_version_meta(self, other: "Output"):
    **ANNOTATION_SCHEMA,
    Output.PARAM_CACHE: bool,
    Output.PARAM_REMOTE: str,
+    Output.PARAM_PULL: vol.All(vol.Lower, Choices("always", "never", "explicit")),


I opted for always and never instead of true and false to avoid confusion with YAML types.

I think it's fine to use YAML types and true / false. Also, I don't think we need the third option. I would make false behavior that same as explicit to keep things simpler.

codecov · 2024-06-25T20:54:05Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.41%. Comparing base (8f8d49d) to head (e8b3535).
Report is 9 commits behind head on main.

Files	Patch %	Lines
dvc/repo/fetch.py	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10451      +/-   ##
==========================================
+ Coverage   90.34%   90.41%   +0.07%     
==========================================
  Files         481      481              
  Lines       35981    36036      +55     
  Branches     5038     5053      +15     
==========================================
+ Hits        32506    32583      +77     
+ Misses       2877     2873       -4     
+ Partials      598      580      -18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dvc/repo/fetch.py

shcheklein

We also need a dvc.org PR / update. Thanks @pbailey-hf !

pbailey-hf · 2024-06-27T18:16:55Z

We also need a dvc.org PR / update. Thanks @pbailey-hf !

Done! iterative/dvc.org#5263

shcheklein · 2024-06-28T18:10:37Z

@skshetry if everything looks good, can we merge this? can you do some final checks please?

tests/func/test_checkout.py

skshetry · 2024-07-04T03:09:29Z

tests/func/test_checkout.py

+    assert not (tmp_dir / "explicit_2").exists()
+    assert (tmp_dir / "always").read_text() == "z"
+
+    dvc.checkout(targets="explicit_1.dvc", force=True)


@dberenbaum @shcheklein, this looks odd to me, that a pull option is affecting checkout.

I don't think we should support this at all.

If we don't support it, checkout will fail if we haven't fetched it already as before with the following message:

dvc.exceptions.CheckoutError: Checkout failed for following targets: explicit_1 explicit_2 Is your cache up to date? <https://error.dvc.org/missing-files>

pull() should work as expected though, and not fail.

@skshetry I understand your concern, what is the practical thing here for users though? any idea? If you have a few targets that have pull: false you will keep hitting this failure? do we need an explicit option for checkout then - something like "--allow-missing" in dvc pull?

what is the expected behavior for dvc pull itself considering that it is dvc fetch + dvc checkout?

probably they should be aligned ...

That was my intuition as well, that pull = fetch + checkout, so the option should apply to both.

Even though it is a combination of fetch and checkout, it does not necessarily mean pull options should apply directly to fetch and checkout. pull is a higher-level command.

@skshetry do, what is your practical suggestion here?

an alternative, re the name: sync: push | pull | both | all | checkout - it can be a single option that pretty much encompasses everything

I think this is reasonable and aligns with what I understood explicit to be trying to do - skip syncing those outputs unless you explicitly pass them as targets (so dvc push/pull/checkout skips it but dvc push/pull/checkout skipped_target doesn't).

I'm not sure we need all of push | pull | both | all | checkout now because we already have push: false and can't deprecate it right away. We could try a minimal version of sync: true | false to start and see with more time which of push | pull | both | checkout are needed.

@dberenbaum

We could try a minimal version of sync: true | false to start

And we would avoid all the operation on it? push, pull, checkout? I think that's fine. I think we can keep granular push: false and even probably fetch: false (see below). People will be asking about this. sync is too broad and won't be convenient in a lot of cases. But I'm fine to start there. @skshetry what is your take?

Would you be open to implicit "allow missing" behavior for pull:false outputs?

It should be fetch: false (not pull:false to emphasize that if affects only fetch. And checkout --allow-missing behaves the same way as it does now (it should not be trying to analyze lower level fetch flags, etc). This way pull and checkout are consistent in their behavior - that is important here.

I think we can add a config option for checkout to allow missing by default.

would that combination work for you?

fetch: false

config option to imply allow-missing?

@pbailey-hf What do you think about sync: false? If you are okay starting there, please go ahead with it, and if you want, you could also add fetch: false or other granular options.

@dberenbaum I'd prefer to control pull/push independently, which makes me lean away from the sync: false option. If I'm picking from all of the options presented so far, I'd prefer fetch:false combined with a configured allow-missing.

Okay, thanks for the feedback and your patience @pbailey-hf! I worry about the complexity of so many flags to configure, but let's go with your preference for now to move forward. Please ping when you would like another review.

pbailey-hf mentioned this pull request Jun 6, 2024

Support pulling named subsets of data, or excluding files from pull #2825

Closed

pbailey-hf force-pushed the explicit-fetch branch from 289b8b1 to 6ae7846 Compare June 14, 2024 19:17

pbailey-hf mentioned this pull request Jun 14, 2024

add description of the pull option to outs in .dvc files iterative/dvc.org#5263

Open

pbailey-hf marked this pull request as ready for review June 14, 2024 19:57

dberenbaum reviewed Jun 21, 2024

View reviewed changes

dberenbaum requested a review from skshetry June 24, 2024 20:10

dberenbaum approved these changes Jun 24, 2024

View reviewed changes

pbailey-hf commented Jun 25, 2024

View reviewed changes

skshetry reviewed Jun 27, 2024

View reviewed changes

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

shcheklein approved these changes Jun 27, 2024

View reviewed changes

skshetry reviewed Jul 1, 2024

View reviewed changes

tests/func/test_checkout.py Outdated Show resolved Hide resolved

skshetry reviewed Jul 1, 2024

View reviewed changes

tests/func/test_checkout.py Outdated Show resolved Hide resolved

pbailey-hf and others added 10 commits July 4, 2024 08:46

add option to require explicit fetch/checkout

395dd9b

simplify filter

690259b

update unit test to cover multiple explicit outs

3238f2f

switch option name to "pull"

d10a3d0

simplify pull option

473cf91

handle targets==None

8d4e103

handle reuse of _collect_indexes in pull context

98a9d18

add fetch test and simplify

9a0bc23

fix test

15033f9

add support for stages

0935bbe

skshetry force-pushed the explicit-fetch branch from ed4eef0 to 0935bbe Compare July 4, 2024 03:01

skshetry reviewed Jul 4, 2024

View reviewed changes

remove pull option from checkout but allow missing for pull=False

514b15e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to require explicit fetch/checkout #10451

add option to require explicit fetch/checkout #10451

pbailey-hf commented Jun 6, 2024 •

edited

Loading

dberenbaum Jun 21, 2024

pbailey-hf Jun 24, 2024

dberenbaum left a comment

shcheklein commented Jun 24, 2024

pbailey-hf commented Jun 25, 2024

dberenbaum commented Jun 25, 2024 •

edited

Loading

pbailey-hf commented Jun 25, 2024

pbailey-hf Jun 25, 2024

shcheklein Jun 25, 2024

codecov bot commented Jun 25, 2024 •

edited

Loading

shcheklein left a comment

pbailey-hf commented Jun 27, 2024

shcheklein commented Jun 28, 2024

skshetry Jul 4, 2024 •

edited

Loading

shcheklein Jul 4, 2024

pbailey-hf Jul 5, 2024

skshetry Jul 5, 2024 •

edited

Loading

shcheklein Jul 5, 2024

dberenbaum Jul 12, 2024

shcheklein Jul 14, 2024

dberenbaum Jul 17, 2024

pbailey-hf Jul 22, 2024

dberenbaum Jul 23, 2024

add option to require explicit fetch/checkout #10451

Are you sure you want to change the base?

add option to require explicit fetch/checkout #10451

Conversation

pbailey-hf commented Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dberenbaum left a comment

Choose a reason for hiding this comment

shcheklein commented Jun 24, 2024

pbailey-hf commented Jun 25, 2024

dberenbaum commented Jun 25, 2024 • edited Loading

pbailey-hf commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

shcheklein left a comment

Choose a reason for hiding this comment

pbailey-hf commented Jun 27, 2024

shcheklein commented Jun 28, 2024

skshetry Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbailey-hf commented Jun 6, 2024 •

edited

Loading

dberenbaum commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

skshetry Jul 4, 2024 •

edited

Loading

skshetry Jul 5, 2024 •

edited

Loading