Skip to content

Conversation

@karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Aug 9, 2021

fix #5676

  1. remove all the experiments in workspace
  2. add tests for it

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@karajan1001 karajan1001 requested a review from a team as a code owner August 9, 2021 01:39
@karajan1001 karajan1001 requested a review from skshetry August 9, 2021 01:39
@pmrowla pmrowla self-requested a review August 9, 2021 02:07
Comment on lines 24 to 25
Copy link
Contributor

Choose a reason for hiding this comment

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

This will remove all experiments in the user's entire git repository. exp remove --workspace should only remove experiments that are based on the user's current HEAD (and not experiments based on other git commits)

#5676 (comment)

so we can't use the general exp_refs() iterator here, it should be:

        head = repo.scm.get_ref()
        exp_refs_by_baseline(head)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to leave the behavior as is and rename the flag to --all. That fits the requirement here and is easier to explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karajan1001 can you please rename the flag to --all? If it is left as --workspace it will be confusing, since all other DVC commands use --workspace to mean HEAD + whatever is in the actual working directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmrowla , @dberenbaum
I think the feature --all is not as useful as --workspace? Can add it sometime later?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think which is more useful depends. Most of the time, I would guess users won't notice a different between them. If they have experiments in other commits, they might want to keep those, but I think it's just as likely that they have forgotten about them or don't know they still exist and would be happy to drop them.

After the discussion in this ticket, I prefer --all because:

  1. It's easier to explain and understand. It's not obvious what is a "workspace" experiment, and users might want to confirm that these are the experiments they want to remove.
  2. It parallels dvc exp list. If we eventually want to do something like dvc exp remove --remote, having this parallel dvc exp list seems useful since it's easy to see what experiments will be deleted.
  3. Similarly, in the future we could add the --rev option like in dvc exp list. This is probably more useful than the --workspace/--all-branches/--all-tags/--all-commits flags since --rev is more granular (and --rev HEAD can do the same as --workspace), and I don't see --all-branches or --all-tags ever being useful in dvc exp remove.

@karajan1001 karajan1001 requested a review from pmrowla August 10, 2021 07:53
@karajan1001
Copy link
Contributor Author

asciicast

@skshetry skshetry removed their request for review August 16, 2021 06:24
Co-authored-by: Peter Rowlands (λ³€κΈ°ν˜Έ) <peter@pmrowla.com>
@pmrowla
Copy link
Contributor

pmrowla commented Aug 16, 2021

Also, don't forget that this will need a docs PR

@karajan1001 karajan1001 requested a review from pmrowla August 16, 2021 11:22
Comment on lines 74 to 75
removed = len(repo.experiments.stash)
repo.experiments.stash.clear()
Copy link
Contributor

@pmrowla pmrowla Aug 16, 2021

Choose a reason for hiding this comment

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

I'm not actually sure if -A/--all should affect queued experiments, since we have a separate option for that (--queue). It seems to me that -A should only affect "full" experiments that have already been reproduced

maybe a question for @dberenbaum

Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ€” It seems simpler to explain if it includes the queue, but it's probably more useful if it doesn't since we already have an option to remove the whole queue. Going back to the discussion above, dvc exp list ignores queued experiments, and I think it makes sense to do the same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dberenbaum , Same with -w/--workspace, do not affect queued ones?

table workspace based all based
committed 1 2
queued 3 4

currently --queue removes 3 and 4. After next change --all removes 1 and 2, and workspace? removes 1 and 3 or only 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought we were getting rid of --workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep both of them for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend keeping one, and I prefer --all so we don't have to get into the confusing corner cases like the one you mentioned above. I think when you document the features, you will find it easier if you don't have to document --workspace πŸ˜„ .

As mentioned in the other discussion above, I think you can add --rev later if functionality similar to --workspace is needed.

karajan1001 and others added 3 commits August 16, 2021 20:29
Co-authored-by: Peter Rowlands (λ³€κΈ°ν˜Έ) <peter@pmrowla.com>
Co-authored-by: Peter Rowlands (λ³€κΈ°ν˜Έ) <peter@pmrowla.com>
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

LGTM on the engineering side, but should hold off on merging until @dberenbaum double checks the final usage

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for the lack of clarity on the syntax. Hopefully keeping it simple for now will be helpful, especially when we start expanding usage like in #6006.

@pmrowla pmrowla merged commit 57899ee into treeverse:master Aug 19, 2021
@karajan1001 karajan1001 deleted the fix5676 branch August 19, 2021 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove all experiments in the workspace

3 participants