exp save: Reintroduce implementation based on executor/queue.

[daavoo]

Closes #9058

Ensures behavior of _stash_exp in exp run is matched in exp save without duplicating logic.

The simpler implementation ended up being worse for keeping exp run and exp save behavior in sync.

[codecov]

Codecov Report

Patch coverage: 90.36% and project coverage change: -0.02 ⚠️

Comparison is base (91d3ff0) 92.86% compared to head (c5b673d) 92.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9165      +/-   ##
==========================================
- Coverage   92.86%   92.85%   -0.02%     
==========================================
  Files         458      458              
  Lines       37027    37044      +17     
  Branches     5350     5344       -6     
==========================================
+ Hits        34386    34398      +12     
- Misses       2114     2118       +4     
- Partials      527      528       +1     

Impacted Files	Coverage Δ
dvc/repo/experiments/executor/base.py	83.73% <75.00%> (-0.57%)	⬇️
dvc/repo/experiments/executor/local.py	91.47% <100.00%> (+0.06%)	⬆️
dvc/repo/experiments/save.py	100.00% <100.00%> (+4.76%)	⬆️
tests/func/experiments/test_save.py	100.00% <100.00%> (ø)

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[daavoo]

@dberenbaum I have done some QA and updated the tests but might have missed some discrepancies with exp run , could you give it a sweep?

[dberenbaum]

Can't remember if it's a new issue or whether this should be a separate follow-up, but an untracked dvc.lock is not being included automatically:

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

I also can't remember whether exp run/save are supposed to be preserving the git staging status. Everything staged gets saved, but it gets unstaged after exp run/save:

$ git add .
$ dvc exp run
$ git status
On branch main
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        baz.txt
        dvc.lock

nothing added to commit but untracked files present (use "git add" to track)
$ git add .
$ dvc exp save
$ git status
On branch main
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        baz.txt
        dvc.lock

nothing added to commit but untracked files present (use "git add" to track)

[daavoo]

I don't really know why some tests in this file randomly fail (mostly on windows , few times on macos) when running the full test suite but pass when running the file in isolation:

Failure: https://github.com/iterative/dvc/actions/runs/4498532766/jobs/7915264864#step:6:934
Passing on isolation: https://github.com/iterative/dvc/actions/runs/4499133901/jobs/7916623248

I updated the tests to use a "from scratch" setup and only tmp_dir, dvc, scm fixtures.

[pmrowla]

Suggested change

	save_result = executor.save(
	executor.info, force=force, include_untracked=include_untracked
	)
	result = queue.collect_executor(repo.experiments, executor, save_result)
	try:
	save_result = executor.save(
	executor.info, force=force, include_untracked=include_untracked
	)
	result = queue.collect_executor(repo.experiments, executor, save_result)
	finally:
	executor.cleanup()

[pmrowla]

You need to cleanup the executor here since it has it's own scm instance that isn't getting closed.

Not sure whether or not this is the cause of your flaky tests, but it's possible some git object isn't being written/flushed before your test tries to read it afterwards

[daavoo]

It appeared to be the cause. Thanks!