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

Add remove queued experiment command #1454

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Add remove queued experiment command #1454

merged 2 commits into from
Mar 17, 2022

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Mar 17, 2022

2/2 main <- #1453 <- this

This PR addresses the first point of #1392 OP: "Removal of an experiment from the queue (dvc exp remove [queued_experiment_hash])."

From speaking to Dave about whether or not this should have been a separate command or included with the existing exp remove command:

"queuing is in the middle of big changes. Right now, it makes some sense to keep them together since the same command removes them in the CLI. But we are moving to separate them more, so ultimately it might be better to not view them as just another experiment."

I did try both ways and having them split out did feel more intuitive.

Demo

Screen.Recording.2022-03-17.at.12.31.03.pm.mov

Note: When removing a queued experiment the only file path that is hit is /vscode-dvc/.git/logs/refs/exps/stash I have updated the experiment file system watcher to cater for this.

@mattseddon mattseddon added the product PR that affects product label Mar 17, 2022
@mattseddon mattseddon self-assigned this Mar 17, 2022
@mattseddon mattseddon changed the base branch from main to remove-queued March 17, 2022 00:16
@mattseddon mattseddon changed the base branch from remove-queued to main March 17, 2022 00:23
@mattseddon mattseddon changed the base branch from main to remove-queued March 17, 2022 01:35
@mattseddon mattseddon marked this pull request as ready for review March 17, 2022 01:38
@sroy3
Copy link
Contributor

sroy3 commented Mar 17, 2022

I don't know if it's planned, but at another stage, but maybe also adding an action on the queued experiment in the tree to remove it would be a little more intuitive? Something like the "X" in the sorts and filters maybe?

Base automatically changed from remove-queued to main March 17, 2022 17:23
@mattseddon mattseddon enabled auto-merge (squash) March 17, 2022 17:41
@mattseddon
Copy link
Member Author

mattseddon commented Mar 17, 2022

I don't know if it's planned, but at another stage, but maybe also adding an action on the queued experiment in the tree to remove it would be a little more intuitive? Something like the "X" in the sorts and filters maybe?

I'm not sure if we have a ticket for the tree but we should be able to do this action (and more) from both the tree and the table. It is a destructive action so we will need to be careful and potentially add a warning modal across the board. I will look for a ticket and add one if I cannot find one.

@codeclimate
Copy link

codeclimate bot commented Mar 17, 2022

Code Climate has analyzed commit 20aad91 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 96.3% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.2% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit f4b17ad into main Mar 17, 2022
@mattseddon mattseddon deleted the remove-queued-exp branch March 17, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants