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.

Sign up for GitHub

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

Jump to bottom

Fix #10593 -- add --keep option for dvc experiments remove #10633

Merged

shcheklein merged 25 commits into iterative:main from rmic:fix-10593

Nov 30, 2024

+118 −2

Contributor

rmic commented Nov 23, 2024 •

edited

Loading

Description

This PR addresses issue #10593 where a --keep flag was suggested to keep only the last 'n' experiments (to keep the latest ones after a ton of failed attempts) instead of deleting them

A new flag is introduced on the cli only for the experiments remove command
The code of the command has been adapted to do so
Tests have been written and executed

Checklist

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. : Update documentation for fix #10593 dvc.org#5325

rmic force-pushed the fix-10593 branch from 8560ebd to 3ba8d4e Compare

November 23, 2024 22:27

codecov bot commented Nov 23, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.03%. Comparing base (2431ec6) to head (fe5c5d7).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
dvc/repo/experiments/refs.py	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10633      +/-   ##
==========================================
+ Coverage   90.68%   91.03%   +0.35%     
==========================================
  Files         504      504              
  Lines       39795    39847      +52     
  Branches     3141     3151      +10     
==========================================
+ Hits        36087    36275     +188     
+ Misses       3042     2945      -97     
+ Partials      666      627      -39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rmic marked this pull request as ready for review

November 23, 2024 22:55

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/commands/experiments/__init__.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

Contributor Author

rmic commented Nov 24, 2024

Hi @shcheklein !

Thanks a lot for your review !
I'll make changes according to your comments asap.

rmic force-pushed the fix-10593 branch 3 times, most recently from 8280009 to e17f388 Compare

November 24, 2024 12:44

rmic mentioned this pull request

Fix #5325 : added --keep flag to dvc exp remove documentation iterative/dvc.org#5326

Open

shcheklein reviewed

View reviewed changes

dvc/commands/experiments/__init__.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

dvc/repo/experiments/remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

tests/func/experiments/test_remove.py Outdated Show resolved Hide resolved

shcheklein reviewed

View reviewed changes

tests/func/experiments/test_remove.py Outdated Show resolved Hide resolved

rmic force-pushed the fix-10593 branch 2 times, most recently from f5e8788 to 286b465 Compare

November 28, 2024 18:13

Contributor Author

rmic commented Nov 28, 2024

It looks like the lint test is broken on windows-latest.

This was referenced Nov 28, 2024

dvc exp remove --queue -A: possibly incomplete result returned #10638

Open

dvc exp remove: should we make --queue work with -n and other flags ? #10639

Open

shcheklein reviewed

View reviewed changes

tests/func/experiments/test_remove.py Show resolved Hide resolved

shcheklein approved these changes

View reviewed changes

shcheklein reviewed

View reviewed changes

dvc/commands/experiments/__init__.py Outdated Show resolved Hide resolved

rmic force-pushed the fix-10593 branch from acbd81f to 92aed36 Compare

November 29, 2024 08:45

This comment was marked as resolved.

Sign in to view

rmic added 2 commits

November 29, 2024 21:14


          Add keep_selected parameter, and corresponding code to keep only the …

725b02d

…selected exps (and remove all the other ones)


          test keep_selected_by_name

e47cc72

rmic and others added 23 commits

November 29, 2024 21:14


          test keep_selected_by_rev

df6a7ed


          test keep_selected multiple, by name

df2a1ee


          test keep all by name

4f32f20


          test keep by rev, with num=2

f913ffb


          added option to cli

c3a46da


          refactoring to meet pr needs

4e84b2d


          [pre-commit.ci] auto fixes from pre-commit.com hooks

7f050ed

for more information, see https://pre-commit.ci


          fixed test_experiments to add keep_selected=False to remove tests

36a0e2b


          rename parameter to match cli option

87abac1


          follow the normal path, then invert the selection before removing

ca279b9


          fixed tests for list ordering + fixed test with non existent name, it…

a903507

… didn't make sense to delete everything if an exp name did not exist


          changed cli option comment

cb43ca3


          [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci


          fixed typing issue

dea54cb


          updated parameter name

421c3f0


          removed handling queued experiments (since --queue would remove them …

0a76842

…all)


          [pre-commit.ci] auto fixes from pre-commit.com hooks

783b67a

for more information, see https://pre-commit.ci


          code simplification, added __eq__ and __hash__ to be able to compare …

b83108a

…ExpRefs, updated and parametrized tests.


          [pre-commit.ci] auto fixes from pre-commit.com hooks

61fda46

for more information, see https://pre-commit.ci


          fixed linting issues

c6c67b9


           - --keep and --queue together raise an InvalidArgumentError

9397e83

    - added a test to check if the error is raised
    - fixed CLI message


          [pre-commit.ci] auto fixes from pre-commit.com hooks

9365b8a

for more information, see https://pre-commit.ci


          re-run gh tests. Some tests which did not involve my changes started …

fe5c5d7

…failing while they were passing fine before.

skshetry force-pushed the fix-10593 branch from b127608 to fe5c5d7 Compare

November 29, 2024 15:29

rmic changed the title ~~Fix 10593 -- add --keep option for dvc experiments remove~~ Fix #10593 -- add --keep option for dvc experiments remove

shcheklein merged commit 368c785 into iterative:main

27 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet