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

Enable experiment operations when experiment(s) are running in the queue #3832

Merged
merged 7 commits into from
May 8, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented May 5, 2023

1/3 main <- this <- #3834 <- #3840

Part of #3178

This PR makes it possible to run experiments operations when experiments are running in the queue. Actions will still be disabled on experiments running in the queue and on all experiments when there is an experiment running in the workspace.

Demo

Screen.Recording.2023-05-05.at.1.35.35.pm.mov

@mattseddon mattseddon added the product PR that affects product label May 5, 2023
@mattseddon mattseddon self-assigned this May 5, 2023
@mattseddon mattseddon force-pushed the only-disable-for-workspace-runs branch from f847d56 to cd53016 Compare May 5, 2023 03:24
@mattseddon mattseddon force-pushed the only-disable-for-workspace-runs branch from cd53016 to 2c9eb22 Compare May 5, 2023 04:56
COMMIT = 'commit',
EXPERIMENT = 'experiment',
QUEUED = 'queued'
RUNNING = 'running',
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] We need this to prevent actions being run from the experimentsTree context menu for running experiments.

@@ -664,11 +664,11 @@
},
{
"command": "dvc.applyExperiment",
"when": "dvc.commands.available && dvc.project.available && !dvc.experiment.running"
"when": "dvc.commands.available && dvc.project.available && !dvc.experiment.running.workspace"
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] We only want to suppress actions when there is an experiment running in the workspace.

@@ -1158,42 +1163,42 @@
{
"command": "dvc.views.experiments.applyExperiment",
"group": "inline@1",
"when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem == experiment && !dvc.experiment.running"
"when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem == experiment && !dvc.experiment.running.workspace"
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] viewItem == experiment means none of these actions will be available for running experiments.

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'm going to add stop inline as a follow up

batchRowSelection={batchRowSelection}
/>
)
}) => <NestedRow row={row} batchRowSelection={batchRowSelection} />
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Using redux for this state instead of drilling

status: ExperimentStatus.SUCCESS,
subRows: row.subRows?.map(subRow => ({
...subRow,
status: ExperimentStatus.SUCCESS
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Have to set the status on each experiment for them to count as not running.

@mattseddon mattseddon marked this pull request as ready for review May 8, 2023 02:56
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 8, 2023 02:56
@mattseddon mattseddon requested a review from shcheklein May 8, 2023 02:56
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work!

type?: ExperimentType
selected?: boolean
}): ThemeIcon | Uri | Resource {
if (isRunning(status)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these helpers still useful if they're checking only the type? It was removed here, but used again in extension/src/experiments/model/index.ts. We should stay consistent and either use them everywhere or remove them.

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

codeclimate bot commented May 8, 2023

Code Climate has analyzed commit 2f63386 and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@mattseddon mattseddon merged commit ea310a5 into main May 8, 2023
@mattseddon mattseddon deleted the only-disable-for-workspace-runs branch May 8, 2023 21:12
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