exp save: initial implementation #8599

daavoo · 2022-11-21T17:11:08Z

copy of #8408

Makes it possible to save changes in the current workspace as a dvc experiment.

codecov · 2022-11-21T17:32:41Z

Codecov Report

Base: 93.89% // Head: 94.11% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (4fb79dd) compared to base (8124396).
Patch coverage: 95.07% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8599      +/-   ##
==========================================
+ Coverage   93.89%   94.11%   +0.22%     
==========================================
  Files         432      435       +3     
  Lines       33159    33298     +139     
  Branches     4663     4677      +14     
==========================================
+ Hits        31133    31340     +207     
+ Misses       1582     1528      -54     
+ Partials      444      430      -14

Impacted Files	Coverage Δ
dvc/commands/experiments/__init__.py	`100.00% <ø> (ø)`
dvc/commands/experiments/save.py	`82.14% <82.14%> (ø)`
dvc/repo/experiments/executor/local.py	`90.79% <94.11%> (+2.24%)`	⬆️
dvc/repo/experiments/save.py	`100.00% <100.00%> (ø)`
tests/func/experiments/test_experiments.py	`99.72% <100.00%> (ø)`
tests/func/experiments/test_save.py	`100.00% <100.00%> (ø)`
tests/unit/command/test_experiments.py	`99.59% <100.00%> (+0.01%)`	⬆️
tests/remotes/git_server.py	`33.33% <0.00%> (-9.10%)`	⬇️
dvc/testing/fixtures.py	`89.07% <0.00%> (-6.73%)`	⬇️
dvc/analytics.py	`91.04% <0.00%> (ø)`
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dberenbaum · 2022-11-24T14:26:31Z

Thanks @daavoo!

I'm still seeing issues with files not being saved by the experiment:

# Add stage to dvc.yaml
$ dvc stage add -q -n exp_save -M metrics.yaml 'echo "foo: 1" > metrics.yaml'
# Add random file
$ echo misc > misc
# Generate metrics
$ dvc repro -q
# Track everything with git
$ git add .
# Save experiment
$ dvc exp save -q
# Stash everything from the workspace
$ git stash -u
# Apply the experiment to recover saved files
$ dvc exp apply exp-48145
# Metrics aren't saved
$ dvc exp show
 ────────────────────────────────────
  Experiment                Created
 ────────────────────────────────────
  workspace                 -
  main                      09:14 AM
  └── d453430 [exp-48145]   09:14 AM
 ────────────────────────────────────
# No files were recovered during apply
$ git status
On branch main
nothing to commit, working tree clean

There are 2 issues:

dvc exp run saves metrics.yaml, dvc.yaml, and dvc.lock regardless of whether they are added to git, but dvc exp save loses all of these. It seems like this is needed for it to be useful the first time the experiment is saved.
Neither dvc exp run or dvc exp save saves arbitrary new files, even if they are added to git. This is related to the idea from the previous PR that it should include all untracked files. It might not be a blocker for now if solving the first issue is enough for dvclive, but it seems like there needs to be some mechanism to save new files to an experiment (either all untracked files or at least those that have been added to git).

daavoo · 2022-11-24T17:16:38Z

dvc/repo/experiments/save.py

+    repo: "Repo",
+    name: Optional[str] = None,
+    force: bool = False,
+    include_untracked: Optional[List[str]] = None,


2. Neither dvc exp run or dvc exp save saves arbitrary new files, even if they are added to git. This is related to the idea from the previous PR that it should include all untracked files. It might not be a blocker for now if solving the first issue is enough for dvclive, but it seems like there needs to be some mechanism to save new files to an experiment (either all untracked files or at least those that have been added to git).

@dberenbaum To cover the DVCLive scenario I included this option (which is not exposed in the CLI but I can expose it).

This allows to pass pattern(s) of (potentially) untracked files to run git add internally.

daavoo · 2022-11-24T17:27:39Z

1. `dvc exp run` saves `metrics.yaml`, `dvc.yaml`, and `dvc.lock` regardless of whether they are added to git, but `dvc exp save` loses all of these. It seems like this is needed for it to be useful the first time the experiment is saved.

dvc exp run will not save dvc.lock in the experiment ref if you run the same steps, replacing exp save with exp run. After exp apply you will only see dvc.yaml and metrics.yaml.

It will include in the ref the dvc.yaml because it is needed for executing the pipeline and metrics.yaml because it's produced by the execution pipeline. dvc.lock will be present in the workspace as an untracked file as a result of the execution, but it will not be the dvc.lock produced by repro -q, that one is lost.

I don't mean to dismiss the point, but in exp save there is no execution at all, so the differences in behavior kind of make sense to me. If we want to match exp run, I guess we could default to use include_untracked with dvc.lock, dvc.yaml and all outputs in the pipeline?

In the DVCLive scenario, DVCLive will take care of including the untracked files by #8599 (comment)

In the DVC scenario, I can expose the same option to the CLI, and/or we can be very explicit in docs about the behavior of staged and/or untracked files.

I think the UI is already clear. These are the current warnings:

# staged
$ dvc exp save
WARNING: Your workspace contains staged Git changes which will be unstaged before saving this experiment.
WARNING: The following untracked files were present in the workspace before saving but will not be included in the experiment commit:
	metrics.yaml, dvc.lock, dvc.yaml

# untracked
$ dvc exp save
WARNING: The following untracked files were present in the workspace before saving but will not be included in the experiment commit:
	metrics.yaml, dvc.lock, dvc.yaml

daavoo · 2022-11-25T11:03:22Z

dvc/commands/experiments/save.py

+    save_parser.add_argument(
+        "-I",
+        "--include-untracked",
+        action="append",
+        default=[],
+        help="List of untracked paths to include in the experiment.",
+        metavar="<path>",
+    )


@dberenbaum

$ dvc stage add -q -n exp_save -M metrics.yaml 'echo "foo: 1" > metrics.yaml' $ echo misc > misc $ dvc repro -q $ dvc exp save -I "misc" -I "metrics.yaml" -I "dvc.lock" -I "dvc.yaml" $ git stash -u $ dvc exp apply exp-48145 $ git status On branch master Untracked files: (use "git add <file>..." to include in what will be committed) dvc.lock dvc.yaml metrics.yaml misc $ dvc exp show dvc exp show ────────────────────────────────────────── Experiment Created foo ────────────────────────────────────────── workspace - 1 master 11:57 AM - └── 71d7388 [exp-48145] 11:58 AM 1 ──────────────────────────────────────────

In DVCLive I am hardcoding the option to include_untracked=self.dir

dberenbaum

@daavoo I'll open a follow-up issue to discuss how to handle untracked files. For now, as long as it works with DVCLive, let's merge it. Having a public CLI command isn't even a requirement right now.

Remove metrics. Move tests to separate file. Fix `experiments.get_exact_name` usage. Remove unused checkpoint logic.

Staged changes were causing a merge error. Warn and unstage instead of erroring, to match exp run behavior.

Allow to include a list of potentially untracked files. Covers the DVCLive use case where in the first run the generated files won't be tracked in Git yet.

dtrifiro · 2022-11-28T14:36:01Z

dvc/repo/experiments/executor/local.py

+            )
+            ref: Optional[str] = dvc.scm.get_ref(EXEC_BRANCH, follow=False)
+            exp_ref = ExpRefInfo.from_ref(ref) if ref else None
+            untracked = dvc.scm.untracked_files()


Just a note: this check might slow down exp save considerably when large untracked directories are present, but I guess it's fine for now

dtrifiro · 2022-11-28T14:39:15Z

dvc/repo/experiments/save.py

+    queue = repo.experiments.workspace_queue
+    logger.debug("Saving workspace in %s", os.getcwd())
+
+    staged, _, _ = repo.scm.status(untracked_files="no")


Since we're checking for untracked files above, how about:

Suggested change

staged, _, _ = repo.scm.status(untracked_files="no")

staged, unstaged,untracked = repo.scm.status(untracked_files="no")

if untracked:

logger.warning(

"The following untracked files were present in "

"the workspace before saving but "

"will not be included in the experiment commit:\n"

"\t%s",

", ".join(untracked),

)

Let's make this change and remove the other call, but we need to handle include_untracked

dberenbaum · 2022-11-29T14:23:18Z

@daavoo @dtrifiro Can you open a docs ticket for this?

daavoo mentioned this pull request Nov 21, 2022

[WIP] exp save: initial implementation #8408

Closed

daavoo marked this pull request as draft November 21, 2022 17:17

daavoo force-pushed the 8408 branch from 0184459 to 5ceffa5 Compare November 21, 2022 17:19

daavoo force-pushed the 8408 branch 4 times, most recently from 4d6520e to a7dc375 Compare November 24, 2022 12:16

daavoo marked this pull request as ready for review November 24, 2022 12:17

daavoo requested a review from dberenbaum November 24, 2022 12:17

daavoo self-assigned this Nov 24, 2022

daavoo added A: experiments Related to dvc exp feature is a feature labels Nov 24, 2022

daavoo mentioned this pull request Nov 24, 2022

Create DVC experiment on live.end. iterative/dvclive#366

Merged

daavoo commented Nov 24, 2022

View reviewed changes

daavoo force-pushed the 8408 branch 3 times, most recently from e33034e to bcf3f66 Compare November 25, 2022 11:01

daavoo commented Nov 25, 2022

View reviewed changes

dberenbaum approved these changes Nov 25, 2022

View reviewed changes

dberenbaum mentioned this pull request Nov 25, 2022

exp run/save: untracked files #8629

Closed

daavoo enabled auto-merge (rebase) November 28, 2022 12:59

dtrifiro and others added 3 commits November 28, 2022 14:17

exp save: initial implementation

4c7efb4

exp save: cleanup.

54bd87c

Remove metrics. Move tests to separate file. Fix `experiments.get_exact_name` usage. Remove unused checkpoint logic.

exp: save: Handle staged changes.

86a0f50

Staged changes were causing a merge error. Warn and unstage instead of erroring, to match exp run behavior.

daavoo force-pushed the 8408 branch 2 times, most recently from 6d61790 to 1144cfa Compare November 28, 2022 13:22

exp: save: Add include_untracked.

4fb79dd

Allow to include a list of potentially untracked files. Covers the DVCLive use case where in the first run the generated files won't be tracked in Git yet.

daavoo force-pushed the 8408 branch from 1144cfa to 4fb79dd Compare November 28, 2022 14:24

daavoo requested a review from dtrifiro November 28, 2022 14:25

dtrifiro reviewed Nov 28, 2022

View reviewed changes

daavoo merged commit a036eaf into main Nov 28, 2022

daavoo deleted the 8408 branch November 28, 2022 14:49

dberenbaum mentioned this pull request Nov 29, 2022

dvc exp save action iterative/vscode-dvc#2856

Closed

dtrifiro mentioned this pull request Nov 30, 2022

add docs for dvc exp save iterative/dvc.org#4148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exp save: initial implementation #8599

exp save: initial implementation #8599

daavoo commented Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

dberenbaum commented Nov 24, 2022

daavoo Nov 24, 2022

daavoo commented Nov 24, 2022 •

edited

Loading

daavoo Nov 25, 2022

daavoo Nov 25, 2022

dberenbaum left a comment

dtrifiro Nov 28, 2022

dtrifiro Nov 28, 2022

daavoo Nov 28, 2022

dberenbaum commented Nov 29, 2022

-    staged, _, _ = repo.scm.status(untracked_files="no")
+    staged, unstaged,untracked = repo.scm.status(untracked_files="no")
+    if untracked:
+        logger.warning(
+            "The following untracked files were present in "
+            "the workspace before saving but "
+            "will not be included in the experiment commit:\n"
+             "\t%s",
+             ", ".join(untracked),
+        )

exp save: initial implementation #8599

exp save: initial implementation #8599

Conversation

daavoo commented Nov 21, 2022 • edited Loading

codecov bot commented Nov 21, 2022 • edited Loading

Codecov Report

dberenbaum commented Nov 24, 2022

daavoo Nov 24, 2022

Choose a reason for hiding this comment

daavoo commented Nov 24, 2022 • edited Loading

daavoo Nov 25, 2022

Choose a reason for hiding this comment

daavoo Nov 25, 2022

Choose a reason for hiding this comment

dberenbaum left a comment

Choose a reason for hiding this comment

dtrifiro Nov 28, 2022

Choose a reason for hiding this comment

dtrifiro Nov 28, 2022

Choose a reason for hiding this comment

Since we're checking for untracked files above, how about:

daavoo Nov 28, 2022

Choose a reason for hiding this comment

dberenbaum commented Nov 29, 2022

daavoo commented Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

daavoo commented Nov 24, 2022 •

edited

Loading