exp run: track files for skipped/run-cached stages

[pmrowla]

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

Fixes #9781

[pmrowla]

stage.reproduce() returns None for skipped (unchanged run-cached) stages, and when the entire pipeline is skipped dvc.reproduce() ends up returning an empty list. In exp run we still need to force git-tracking the dvc.yaml and dvc.lock for those skipped stages.

[codecov]

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage
dvc/repo/experiments/executor/base.py	100.00%

📢 Thoughts on this report? Let us know!.

[skshetry]

@pmrowla, what do you think of changing the return type of dvc.reproduce()?

[pmrowla]

@pmrowla, what do you think of changing the return type of dvc.reproduce()?

I thought about having it return the tuple of ([reproduced], [skipped]) stages but wasn't sure if that would be useful anywhere else outside of this use case

[skshetry]

@pmrowla, what do you think of changing the return type of dvc.reproduce()?

I thought about having it return the tuple of ([reproduced], [skipped]) stages but wasn't sure if that would be useful anywhere else outside of this use case

I think it should be something like following now, after --keep-going and --ignore-errors implementation.

@dataclass
class Result:
    reproduced: List["Stage"] = field(default_factory=list)
    unchanged: List["Stage"] = field(default_factory=list)
    failed: List["Stage"] = field(default_factory=list)
    skipped: List["Stage"] = field(default_factory=list)

I think we are long due for changing return type of repro. We have been hacking just to not change the return type.

But I guess adjusting tests will be painful, so I'm happy with this too for now. But we'll have to change the return type at some point.

[skshetry]

I am also okay with introducing a post_run_hook as that's what this seems to be about, and will avoid wrapping _reproduce_stage.

[skshetry]

Btw, what should be the behaviour here if dvc exp run fails? Should it still stage files from stages that were run? Previous implementation did not do that, this one does.

[daavoo]

I agree changing the return type would be cleaner but no strong opinion or need to block the P.R.

[pmrowla]

Btw, what should be the behaviour here if dvc exp run fails? Should it still stage files from stages that were run? Previous implementation did not do that, this one does.

If it fails, we don't make a commit and everything gets git reset to the state prior to the experiment anyways, so it doesn't make a difference whether or not we stage anything right now

[pmrowla]

I think we're all in agreement that the return type for reproduce() will need to be reworked going forward, but I think that's beyond the scope of this bug fix, especially since it will require updating a lot of tests

Going to go ahead and merge this as-is for now