Skip to content

Conversation

@karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Jan 17, 2022

fix: #7155

in exp remove

  1. rename flags -A/--all-commits
  2. add two new flags rev and num
  3. unify the revision collection
  4. add unit and func tests for them

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

wait for #7245

@karajan1001 karajan1001 requested a review from a team as a code owner January 17, 2022 10:15
@karajan1001 karajan1001 requested a review from efiop January 17, 2022 10:15
@karajan1001 karajan1001 changed the title exp remove: rename --all-commits flag add a new rev flag and unify the collection of revs (#7155) [WIP] exp remove: rename --all-commits flag add a new rev flag and unify the collection of revs (#7155) Jan 17, 2022
@karajan1001 karajan1001 marked this pull request as draft January 17, 2022 10:15
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 17, 2022
@karajan1001 karajan1001 requested review from pmrowla and removed request for efiop January 18, 2022 07:01
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 18, 2022
@karajan1001 karajan1001 force-pushed the fix7155 branch 3 times, most recently from 7f6ea98 to 893139d Compare January 24, 2022 07:54
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 24, 2022
@karajan1001 karajan1001 force-pushed the fix7155 branch 2 times, most recently from cb36257 to 6414c1c Compare January 25, 2022 12:05
@karajan1001
Copy link
Contributor Author

@dberenbaum
One problem here, in previous --all only affect on committed exps while --queue affect only on queued ones.
But for my current implements dvc exp remove --rev master will delete all of the exps whose baseline is master.
I guess it is better to unify their behaviours here?

asciicast

@dberenbaum
Copy link
Contributor

Should an error be thrown for dvc exp remove with no arguments, similar to what we do for dvc exp gc?

@dberenbaum
Copy link
Contributor

One problem here, in previous --all only affect on committed exps while --queue affect only on queued ones.
But for my current implements dvc exp remove --rev master will delete all of the exps whose baseline is master.
I guess it is better to unify their behaviours here?

Are dvc exp show and dvc exp run the only other commands that use the queue? I think either of these behaviors is acceptable:

  • Remove queued experiments from the specified revisions only when --queue is added. So dvc exp remove --rev master -n 2 should not drop any queued experiments, but dvc exp remove --rev master --queue -n 2 should remove queued experiments from master and the prior commit. dvc exp remove -A --queue should remove all queued experiments.
  • Keep --queue separate and not interact with any other flags. So dvc exp remove --rev master should not drop any queued experiments, and dvc exp remove --queue should drop queued experiments from all commits.

@pmrowla Any thoughts as you work through queuing/tasks?

Note that the behavior currently seems pretty inconsistent with respect to queued experiments:

$ dvc exp show -n 2
 ────────────────────────────────────────────────────────────────────────────────────────
  Experiment                  Created        State    avg_prec   roc_auc   prepare.split
 ────────────────────────────────────────────────────────────────────────────────────────
  workspace                   -              -         0.60724   0.96022   0.2
  master                      12:55 PM       -         0.60724   0.96022   0.2
  ├── 79d0145                 01:03 PM       Queued          -         -   0.2
  random-forest-experiments   May 29, 2021   -         0.60405    0.9608   0.2
 ────────────────────────────────────────────────────────────────────────────────────────
$ dvc exp remove -A
$ dvc exp show -n 2
 ────────────────────────────────────────────────────────────────────────────────────────
  Experiment                  Created        State    avg_prec   roc_auc   prepare.split
 ────────────────────────────────────────────────────────────────────────────────────────
  workspace                   -              -         0.60724   0.96022   0.2
  master                      12:55 PM       -         0.60724   0.96022   0.2
  ├── 79d0145                 01:03 PM       Queued          -         -   0.2
  random-forest-experiments   May 29, 2021   -         0.60405    0.9608   0.2
 ────────────────────────────────────────────────────────────────────────────────────────
$ dvc exp remove -n 2
$ dvc exp show -n 2
 ────────────────────────────────────────────────────────────────────────────────────────
  Experiment                  Created        State    avg_prec   roc_auc   prepare.split
 ────────────────────────────────────────────────────────────────────────────────────────
  workspace                   -              -         0.60724   0.96022   0.2
  master                      12:55 PM       -         0.60724   0.96022   0.2
  ├── 79d0145                 01:03 PM       Queued          -         -   0.2
  random-forest-experiments   May 29, 2021   -         0.60405    0.9608   0.2
 ────────────────────────────────────────────────────────────────────────────────────────
$ dvc exp remove --rev HEAD -n 2
 ────────────────────────────────────────────────────────────────────────────────────────
  Experiment                     Created        avg_prec   roc_auc   prepare.split   prep
 ────────────────────────────────────────────────────────────────────────────────────────
  workspace                      -               0.60724   0.96022   0.2             2017
  master                         12:55 PM        0.60724   0.96022   0.2             2017
  11-random-forest-experiments   May 29, 2021    0.60405    0.9608   0.2             2017
 ────────────────────────────────────────────────────────────────────────────────────────

@pmrowla
Copy link
Contributor

pmrowla commented Jan 26, 2022

Keep --queue separate and not interact with any other flags. So dvc exp remove --rev master should not drop any queued experiments, and dvc exp remove --queue should drop queued experiments from all commits

I think we should just go with this for now. I think eventually exp remove should only affect "finished" (whether successful or failed) experiments, and "removing not-yet-run experiments from the queue" will be done by a separate command.

@jorgeorpinel

This comment was marked as resolved.

karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 27, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 27, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 27, 2022
@dtrifiro dtrifiro added the A: experiments Related to dvc exp label Feb 1, 2022
@karajan1001 karajan1001 force-pushed the fix7155 branch 2 times, most recently from 6fede20 to 9c9e4a5 Compare February 10, 2022 07:54
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Feb 16, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Feb 16, 2022
@karajan1001 karajan1001 marked this pull request as ready for review March 8, 2022 12:05
@karajan1001 karajan1001 changed the title [WIP] exp remove: rename --all-commits flag add a new rev flag and unify the collection of revs (#7155) exp remove: rename --all-commits flag add a new rev flag and unify the collection of revs (#7155) Mar 8, 2022
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI strings ✅ thanks

@karajan1001 karajan1001 enabled auto-merge (rebase) March 9, 2022 08:26
@karajan1001 karajan1001 disabled auto-merge March 9, 2022 08:28
…evs (treeverse#7155)

fix: treeverse#7155
1. rename flags `-A/--all-commits` in `exp remove`
2. add new flag `-n/--num` in `exp remove`
3. unify the revision collection in `exp remove`
4. add unit and func tests for `exp remove`

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@karajan1001 karajan1001 enabled auto-merge (rebase) March 9, 2022 09:48
@karajan1001 karajan1001 merged commit efc021d into treeverse:main Mar 9, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Mar 15, 2022
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Apr 11, 2022
* exp remove: add new flags `--rev` and `-n`

related to treeverse/dvc#7275

* Some update

* Update content/docs/command-reference/exp/remove.md

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

* Restyled by prettier

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
snorkelopstesting1-a11y pushed a commit to snorkel-marlin-repos/iterative_dvc_pr_7275_cd3ef583-2e08-4fc2-92d4-33997942d7a7 that referenced this pull request Oct 22, 2025
…y the collection of revs (#7155)

Original PR #7275 by karajan1001
Original: treeverse/dvc#7275
snorkelopstesting3-bot added a commit to snorkel-marlin-repos/iterative_dvc_pr_7275_cd3ef583-2e08-4fc2-92d4-33997942d7a7 that referenced this pull request Oct 22, 2025
…new `rev` flag and unify the collection of revs (#7155)

Merged from original PR #7275
Original: treeverse/dvc#7275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: experiments Related to dvc exp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

make all the flags in exp show and exp list support remove

5 participants