Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exp: stop checking for unchanged experiments #8743

Merged
merged 2 commits into from
Jan 4, 2023
Merged

exp: stop checking for unchanged experiments #8743

merged 2 commits into from
Jan 4, 2023

Conversation

dberenbaum
Copy link
Contributor

Following up on iterative/dvc.org#4200 (comment):

Or, if nothing
has changed in the workspace, force saving an experiment anyway (identical to
Git HEAD).

@dtrifiro I missed this check in #8690. I don't think --force should be needed and it complicates this option. Can we drop it?

Ideally, we want to make dvc exp run the same and not worry about creating duplicate experiments (see the discussion in #8659 (comment)).

@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Base: 93.52% // Head: 93.53% // Increases project coverage by +0.00% 🎉

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8743   +/-   ##
=======================================
  Coverage   93.52%   93.53%           
=======================================
  Files         457      457           
  Lines       36282    36268   -14     
  Branches     5262     5258    -4     
=======================================
- Hits        33933    33923   -10     
+ Misses       1843     1838    -5     
- Partials      506      507    +1     
Impacted Files Coverage Δ
dvc/repo/experiments/exceptions.py 79.62% <ø> (-1.41%) ⬇️
dvc/repo/experiments/__init__.py 87.68% <100.00%> (-0.41%) ⬇️
dvc/repo/experiments/executor/base.py 83.47% <100.00%> (+0.70%) ⬆️
dvc/repo/experiments/save.py 94.59% <100.00%> (-0.28%) ⬇️
tests/func/experiments/test_checkpoints.py 100.00% <100.00%> (ø)
tests/func/experiments/test_experiments.py 99.74% <100.00%> (+<0.01%) ⬆️
tests/func/experiments/test_save.py 100.00% <100.00%> (ø)
tests/func/experiments/test_show.py 99.40% <100.00%> (+0.59%) ⬆️
dvc/repo/experiments/queue/celery.py 86.19% <0.00%> (-1.87%) ⬇️
dvc/repo/experiments/queue/workspace.py 81.39% <0.00%> (-0.78%) ⬇️
... and 3 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
Copy link
Contributor Author

Need to fix tests

@dberenbaum
Copy link
Contributor Author

One consequence of this is that there will often be an extra commit for checkpoints experiments. Before this PR, if I run a checkpoints experiment, DVC will create an additional commit at the end of the experiment only if there is a change from the last checkpoint. After this PR, DVC will always create an additional commit at the end of the experiment. I don't see this as a problem -- we are trading off some additional noise and duplication for a simpler approach that's more consistent and transparent.

cc @pmrowla @daavoo

@dberenbaum
Copy link
Contributor Author

This PR also drops reset-specific logic when checking for conflicts. This is no longer needed since we randomly assign a new experiment name at each reset instead of overwriting the previous experiment. This partially addresses #8742.

@dberenbaum
Copy link
Contributor Author

@pmrowla It looks like empty commits are generated for the final state of checkpoints with this PR. The experiment rows are identical and the git diff is empty.

$ dvc exp show
 ──────────────────────────────────────────────────────────────────────────────────────>
  Experiment                 Created        train.loss   train.acc   test.loss   test.a>
 ──────────────────────────────────────────────────────────────────────────────────────>
  workspace                  -                 0.67347     0.78732     0.66219     0.79>
  main                       Dec 15, 2022       0.9731      0.7694     0.95962     0.77>
  │ ╓ cfa235e [alive-bias]   12:03 PM          0.67347     0.78732     0.66219     0.79>
  │ ╟ a52327a                12:03 PM          0.67347     0.78732     0.66219     0.79>
  │ ╟ 952884b                12:03 PM          0.66759     0.79247     0.66191     0.78>
  │ ╟ d028ae0                12:03 PM          0.70363     0.78107     0.69235     0.78>
...
$ git diff cfa235e a52327a

@pmrowla
Copy link
Contributor

pmrowla commented Jan 4, 2023

@pmrowla It looks like empty commits are generated for the final state of checkpoints with this PR. The experiment rows are identical and the git diff is empty.

I think we can just open a separate issue for this and fix it in a follow up PR if needed (not sure it needs to be prioritized right away).

@dtrifiro dtrifiro enabled auto-merge (rebase) January 4, 2023 16:11
@dtrifiro dtrifiro disabled auto-merge January 4, 2023 16:15
@dtrifiro dtrifiro enabled auto-merge (rebase) January 4, 2023 16:20
@dtrifiro dtrifiro merged commit 6ac7dfc into main Jan 4, 2023
@dtrifiro dtrifiro deleted the exp-conflicts branch January 4, 2023 16:29
mattseddon added a commit to iterative/vscode-dvc that referenced this pull request Jan 10, 2023
mattseddon added a commit to iterative/vscode-dvc that referenced this pull request Jan 10, 2023
mattseddon added a commit to iterative/vscode-dvc that referenced this pull request Jan 10, 2023
* update demo project and latest tested CLI version (2.40.0)

* add extra record iterative/dvc#8743
@dberenbaum dberenbaum mentioned this pull request Jan 26, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants