exp run: checks out data dependencies (destructive) [QA]

[jorgeorpinel]

1. It produces a first unchanged exp

UPDATE: Addressed in #5600

$ git clone git@github.com:iterative/example-get-started.git
$ cd example-get-started
$ dvc pull

$ dvc exp run

This works even when there are no changes to the committed project version (HEAD). Below we can see there are differences in metrics or params:

$  dvc exp show --no-pager
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━
┃ Experiment    ┃ Created      ┃ avg_prec ┃ roc_auc ┃ prepare.split ┃ prepare.seed ┃ featurize.max_features ┃ featurize.ngrams ┃ train.seed ┃ train.n_est ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━
│ workspace     │ -            │  0.60405 │  0.9608 │ 0.2           │ 20170428     │ 3000                   │ 2                │ 20170428   │ 100         │
│ master        │ Mar 01, 2021 │  0.60405 │  0.9608 │ 0.2           │ 20170428     │ 3000                   │ 2                │ 20170428   │ 100         │
│ └── exp-44136 │ 02:22 AM     │  0.60405 │  0.9608 │ 0.2           │ 20170428     │ 3000                   │ 2                │ 20170428   │ 100         │
└───────────────┴──────────────┴──────────┴─────────┴───────────────┴──────────────┴────────────────────────┴──────────────────┴────────────┴─────────────┴─

exp diff doesn't print anything.

Is there a use case for this? Otherwise I'd vote to block it.

2. It checks out data dependencies (destructive)

Continuing the previous CLI block

...
$ truncate --size=20M data/data.xml  # This is a stage dep
$ dvc exp run
 33% Checkout|███████████ ...  # I can see this flash momentarily
...
ERROR: Reproduced experiment conflicts with existing experiment 'exp-44136'. To overwrite the existing experiment run:

Even when I changed a dependency, which could be the basis for my experiment, all the pipeline data was checked out again (undoing my manual changes), so this exp is the same as the previous one (1).

BTW if this was the first time I exp run, it would be easy to miss that the data I changed was restored silently in the process. I'd just see that the exp results are the same as in HEAD which would be misleading.

2.2 Does it really behave exactly like repro?

https://dvc.org/doc/command-reference/exp/run reads:

dvc exp run is equivalent to dvc repro for experiments. It has the same behavior when it comes to targets and stage execution

We also say:

Before using this command, you'll probably want to make modifications such as data and code updates...

But when I change an added data file and try exp run, it undoes those changes and reports that no stage has changed. repro re-adds it instead, and then runs any stage downstream.

3. Not all changes to "code" can be queued

Extracted to #5801

In the docs we say "Before running an experiment, you'll probably want to make modifications such as data and code updates, or hyperparameter tuning." Is this the intended behavior?

Because, if we see dvc.yaml as code — and I think we do as we one of DVC's principles is pipeline codification — then this statement isn't completely true when it comes to queueing experiments. It works with regular experiments though, which use the workspace files (not a tmp copy), which makes me think this may be unintended (as we want all kinds of experiments to be consistent in behavior AFAIK).

Specifically, if you create (or modify) dvc.yaml between queued runs and then try to --run-all, you get errors. Example:

$ git init; dvc init
$ git add --all; git commit -m "`dvc -V`"
$ dvc stage add -n hi -o hi "echo hey > hi"
Creating 'dvc.yaml'
Adding stage 'hi' in 'dvc.yaml'
...
$ dvc exp run
... # works

$ dvc exp run --queue
Queued experiment '16c7340' for future execution.
$ dvc stage add -fn hi -o hello "echo hi > hello"
Modifying stage 'hi' in 'dvc.yaml'
...
$ dvc exp run --queue
Queued experiment '41791e2' for future execution.

$ dvc exp run --run-all
!ERROR: 'dvc.yaml' does not exist
ERROR: 'dvc.yaml' does not exist
ERROR: Failed to reproduce experiment '41791e2'
ERROR: Failed to reproduce experiment '16c7340'

[pmrowla]

This works even when there are no changes to the committed project version (HEAD). Below we can see there are differences in metrics or params:

exp diff doesn't print anything.

This is a side effect of experiments being identified by an internal hash of the pipeline. We don't store these hashes for the parent commits (HEAD) so the first run will be treated as a unique experiment.

Storing it as a separate experiment doesn't really hurt anything, its just an empty git commit, no data or anything else is duplicated.

edit: as noted in #5595, for --temp runs in this case we do not create the duplicate experiment. workspace runs should be updated to use the same behavior

Even when I changed a dependency, which could be the basis for my experiment, all the pipeline data was checked out again (undoing my manual changes), so this exp is the same as the previous one (1).

BTW if this was the first time I exp run, it would be easy to miss that the data I changed was restored silently in the process. I'd just see that the exp results are the same as in HEAD which would be misleading.

exp run always starts with a dvc checkout. Prior to the changes in #5586, dvc.lock was always reset to the HEAD state, so experiments would always checkout any dependencies as of HEAD (dropping workspace changes). After that PR is merged, deps will be checked out to their state in your workspace's lock files, so modifications to dependencies should be dvc commit'ed first (so the change is written to dvc.lock or .dvc files) before using exp run.

This is needed for temp directory runs, since we have to dvc checkout the data dependencies in the temp directory. We could consider adjusting behavior so we don't checkout any dependencies in workspace runs, but I don't think we should have different behavior for workspace vs temp dir runs in this case.

[pmrowla]

duplicate empty experiment issue (1) will be fixed in #5600

[jorgeorpinel]

On 2.

deps will be checked out to their state in your workspace's lock files

OK cool, but it can still be destructive. Should we at least warn users when there are changes in dvc status to confirm that modifications to DVC-tracked file/dir deps will be lost?

Cc @dberenbaum

I don't think we should have different behavior for workspace vs temp dir runs

Good point, agreed. An alternative to keep both kinds of runs consistent would be to copy the workspace data to the tmp dirs... But having exps based on HEAD (not the workspace) makes more sense probably. I'd just add that warning.

[dberenbaum]

Should we at least warn users when there are changes in dvc status to confirm that modifications to DVC-tracked file/dir deps will be lost?

This seems reasonable, or suggesting users either dvc commit to keep changes or dvc checkout to recover previous data.

exps based on HEAD

I don't think data dependencies need to be based on HEAD, they just need to be cached and in the dvc.lock file in the workspace so they can be checked out.

[pmrowla]

Good point, agreed. An alternative to keep both kinds of runs consistent would be to copy the workspace data to the tmp dirs... But having exps based on HEAD (not the workspace) makes more sense probably. I'd just add that warning.

The decision to not directly copy data here was for supporting CI use cases and eventual remote (machine) execution. The expected workflow there will be that you need to dvc push data to a DVC remote, and then it will be dvc pull'ed (and checked out) when dvc exp run happens on your remote/CI machine.

We also currently avoid duplicating data for the local tempdir runs (assuming the user is using any non-copy cache link type), which would be another concern with copying workspace data into the tmp dirs.

[jorgeorpinel]

Hey guys just FYI I don't have any more points here. Can we decide what's the ideal solution if any to 2. and 3. above? (3 is new, but connected to 2.)

I'd just add that warning.

Basically the only possible problem of keeping this behavior even with the warning, is that exp run does not behave like repro. Maybe that's not a problem and we just need to update docs about it?

[dberenbaum]

modifications to dependencies should be dvc commit'ed first (so the change is written to dvc.lock or .dvc files) before using exp run

Thanks @jorgeorpinel. It's been awhile since I thought about this issue. Coming back to it now, it's unclear to me if exp run should automatically do a dvc commit first to align with dvc repro. Thoughts?

[jorgeorpinel]

I see what you mean @dberenbaum but repro doesn't git commit so it would still feel pretty different :)

deps will be checked out to their state in your workspace's lock files, so modifications to dependencies should be dvc commit'ed first

@pmrowla did explain how we got here ☝️ but why is that needed? It probably makes sense for --queue/temp runs but I still think regular exps shouldn't change dependencies in the workspace. It's kind of unexpected!