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

Combine/enhance commands used to stop experiments #3840

Merged
merged 8 commits into from
May 9, 2023
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented May 8, 2023

3/3 main <- #3832 <- #3834 <- this

This PR consolidates our "Stop Experiment" commands. There is now no external separation between stopping an experiment running in the workspace or queue. Experiments can now be stopped via an inline action shown in the experiments tree, the tree's view/title, the experiments table context menu, the editor/title and the command palette.

Demo

Screen.Recording.2023-05-09.at.2.41.44.pm.mov
Screen.Recording.2023-05-09.at.2.42.17.pm.mov
Screen.Recording.2023-05-09.at.4.06.48.pm.mov
Screen.Recording.2023-05-09.at.3.03.08.pm.mov

@mattseddon mattseddon added the product PR that affects product label May 8, 2023
@mattseddon mattseddon self-assigned this May 8, 2023
@mattseddon mattseddon changed the base branch from main to enable-extra May 8, 2023 06:44
Base automatically changed from enable-extra to main May 8, 2023 21:35
@mattseddon mattseddon force-pushed the fix-stop-options branch 2 times, most recently from 22e33eb to 176acdf Compare May 9, 2023 04:51
@mattseddon mattseddon marked this pull request as ready for review May 9, 2023 06:11
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 9, 2023 06:11
@mattseddon mattseddon requested a review from shcheklein May 9, 2023 06:11

await collectDvcRootPids(pids, dvcRoot)

const [pid] = [...pids]
Copy link
Contributor

Choose a reason for hiding this comment

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

How about const pid = pids.slice(0, 1)? It doesn't matter one or the other, I guess it's just a preference, but to me it looks like 2 steps vs. one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would still have to convert the set into an array (error TS2339: Property 'slice' does not exist on type 'Set<number>'). Whatever I do it will look a bit whacky. This comment did make me realise that I needed another logic branch "to be safe". Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it now. I thought you were avoiding mutation, I didn't follow to see it was a Set, my bad. Thanks for explaining.

@@ -362,8 +362,8 @@
"category": "DVC"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's just an issue on my end, but when I tried to stop all experimentts when both queed and workspace experiments were running, the plots would fail with an error like:

plots diff adunc-mash mousy-hack terse-scut finer-linn b950796 -o .dvc/tmp/plots --split --json failed with ERROR: unknown Git revision 'adunc-mash'
trim.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, wait looks like this bug is getting fixed in #3846 :)

@mattseddon mattseddon enabled auto-merge (squash) May 9, 2023 20:00
@codeclimate
Copy link

codeclimate bot commented May 9, 2023

Code Climate has analyzed commit 2a21262 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 91.4% (85% is the threshold).

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

View more on Code Climate.

@mattseddon mattseddon merged commit 9198d9c into main May 9, 2023
@mattseddon mattseddon deleted the fix-stop-options branch May 9, 2023 20:20
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