exp apply: git-tracked files which are unchanged from HEAD are not reset on apply

[pmrowla]

originally from discourse: https://discuss.dvc.org/t/cannot-apply-the-first-exp-after-new-commit/1444/3

If you exp apply an experiment which has a git-tracked file that matches the original parent commit, but that file has modifications in your local workspace, the expected version of the file is not applied.

expected behavior:
the file should match the original parent commit (and experiment) version of the file

actual behavior:
the local modifications to the file are kept

This can be reproduced with example-get-started. From a fresh clone with the venv/requirements installed:

# edit params.yaml to have 'max_features: 300'
git add .
git commit -m 'set max_features 300'
dvc exp run
# result exp has an unmodified params.yaml, but modified outputs/metrics
dvc exp run -S featurize.max_features=225
# resulting workspace now has modified params.yaml with 'max_features: 225'
dvc exp apply <result of first run>

the expected behavior now would be that params.yaml has no git changes, and contains max_features: 300, but the actual result is that it contains max_features: 225

[dberenbaum]

Looks like this was introduced in #7910. Seems like it pretty severely breaks exp apply, so marking as p0.

[dberenbaum]

@pmrowla Do we need to stash untracked files? If we keep the stash somewhere so people can recover it (#7955), can we stash without untracked files and skip applying the stash after? Or do we risk losing the untracked files if they conflict with an untracked dep from the experiment? Could we make a commit for the current workspace instead of stashing it to have more control (that might allow to cleanly revert if there were a conflict with untracked files)?

Seems related to #8481 (comment) in that the expectation is to recover the experiment state, not to merge changes.

[pmrowla]

This bug isn't related to untracked files. The issue is that when we re-apply the workspace stash, we normally skip conflicted files, which in most cases means we prefer the applied exp version of a file instead of whatever the user already had in their workspace. The bug here is that in this case, git does not see the stash's version of the params file as conflicting with the exp version.

Basically, in this workflow, the parent commit (HEAD) for both experiments contains max_features: 300. The first experiment contains an unchanged params file (with max_features: 300). The second experiment contains max_features: 225.

When we go to run exp apply exp-1, the user's workspace contains the result of the second experiment as an unstaged git change. This results in a stash commit that says

diff
- max_features: 300
+ max_features: 225

Now, we apply the first experiment, so the user's workspace contains the results of exp 1. For the params file, this means that it still contains max_features: 300 (since the exp-1 version is the same as the parent commit version).

When we go to restore any unrelated changes from the workspace (via stash apply), the diff for the params file still applies cleanly. In almost any other case, the two diffs will conflict with each other, and we just select which version we prefer (i.e. the version from exp-1)

[pmrowla]

Instead of just calling git stash apply/pop at the end, we probably need to iterate over all of the files in the workspace stash commit and individually apply/checkout only the files that do not exist in the applied experiment commit.

[pmrowla]

I think what we want is just to fix both this and #8481 at the same time, and make the new apply workflow something like

# check initial HEAD commit
git stash --include-untracked
git checkout exp-commit
git reset <initial HEAD>
<selectively apply any stashed files that did not exist in exp-commit>

[dberenbaum]

I brought up untracked files because my initial thought was that the easiest way to "selectively apply" was to leave untracked files in the workspace and not apply any stashed files at the end. However, I think it's problematic if the experiment being applied conflicts with those untracked files.

The workflow you proposed LGTM for the most part.

Do we need to selectively apply any files if we keep the stash and give users the info to revert back to their previous workspace? My only concern would be that keeping entries in the stash could be annoying because:

1. It interferes with stash usage for other purposes (for example, git stash pop may be surprising if you don't notice that dvc exp apply added a new stash entry).
2. It could end up creating a lot of stash entries if we add one for every dvc exp apply call.

Ideally, it would be nice to keep only the dirty changes from the last dvc exp apply call and maybe keep them somewhere other than the stash.

[omesser]

Hey folks, sorry to butt in - @dberenbaum 's concerns resonate with me.
"cluttering" the git stash does feel intrusive - will get users to "lose" confidence/usability of git stash if dvc messes with it (and it's a high price to pay).
So, why not commit the changes to some dvc-managed branch pre-apply, instead of stashing?

[pmrowla]

Using the default git stash ref is not a requirement, we already use our own stash refs for other things in DVC experiments. If we want to use our own ref and add another DVC command/flag to revert applies we can do that.

The main point here w/the git stash discussion is so that everyone understands what exp apply is doing (i.e stashing the workspace changes into a git commit that goes onto a stack of other git commits and can be easily popped later). It does not have to mean "explicitly pushing the workspace changes onto the default CLI git stash ref at .git/refs/stash".

[omesser]

Great, thanks for the clarification @pmrowla! what would the experience look like for the users to restore the workspace changes (if they wanted to) if dvc were to use some custom stash ref when applying like you mentioned? Is it git cli friendly or something we would have to guide the user to do via dvc command?

[pmrowla]

We would probably have to add a new DVC command or flag to revert the last exp apply

[dberenbaum]

So it sounds like we should move forward with #8481 using custom refs? If we can keep them somewhere that's not too crazy like .git/refs/exps/stash or similar, it would still be not too painful for advanced users to use them with git cli.

[shcheklein]

Is still p0?

[dberenbaum]

This is taking longer because of the shifted focus to cloud versioning and because we are going for changing the behavior here along with #8481 rather than a strict bug fix, but it's still urgent. @pmrowla When you get back, let's discuss whether you can offload either this issue or remaining cloud versioning work to someone else so we can prioritize this.

[pmrowla]

This will be resolved by the changes for #8481 (as described in my earlier comment)

I think what we want is just to fix both this and #8481 at the same time, and make the new apply workflow something like

# check initial HEAD commit
git stash --include-untracked
git checkout exp-commit
git reset <initial HEAD>
<selectively apply any stashed files that did not exist in exp-commit>