Add experiment run reset to menus #1719

mattseddon · 2022-05-16T09:57:46Z

Relates to #1711

Screenshots

shcheklein

It looks good to me. Thinking more about this - I think we are in a better position to split those more explicitly and do similar thing to any IDEs - Run and Resume buttons instead of "Run (which is actually resume)" + "Reset and Run (which is just regular Run usually)". The current semantics is confusing, I'm making mistakes (granted that for some people that default semantics is better) - @dberenbaum @daavoo wdyt? The only downside is that it won't look and behave the same as in DVC :(

mattseddon · 2022-05-17T01:35:30Z

It looks good to me. Thinking more about this - I think we are in a better position to split those more explicitly and do similar thing to any IDEs - Run and Resume buttons instead of "Run (which is actually resume)" + "Reset and Run (which is just regular Run usually)". The current semantics is confusing, I'm making mistakes (granted that for some people that default semantics is better) - @dberenbaum @daavoo wdyt? The only downside is that it won't look and behave the same as in DVC :(

WDYT about leaving this for now and making a breaking change in DVC? We would have two commands:

dvc exp run
dvc exp resume

The --reset flag could be dropped altogether.

We could then follow up to match the new commands.

For me it is important to match the DVC terminology but happy to do whatever is decided 👍🏻 .

[Q] Which command is actually more popular? Run or resume?

dberenbaum · 2022-05-17T17:18:44Z

WDYT about leaving this for now and making a breaking change in DVC? We would have two commands:
1. `dvc exp run`

2. `dvc exp resume`

dvc exp resume existed in early versions of experiments exactly like you describe 😅. I can't remember why it changed, but I don't think the UI was noticeably better either way.

[Q] Which command is actually more popular? Run or resume?

We don't see a lot of usage of checkpoints, so hard to say. Both are essential to the workflow.

I think we are in a better position to split those more explicitly and do similar thing to any IDEs - Run and Resume buttons instead of "Run (which is actually resume)" + "Reset and Run (which is just regular Run usually)".

I don't think "Run" by itself is helpful with checkpoints because both reset and resume are running experiments. Not sure if separate resume and reset commands make sense in CLI, but I think we should use them in the VS Code menus.

For the menus, I would suggest:

Resume Experiment
Reset and Run Experiment
Run All Queued Experiments [should this be in a separate section below since it's about all queued experiments?]
--
Modify Experiment Param(s) and Resume
Modify Experiment Param(s), Reset, and Run
Modify Experiment Param(s), Reset, and Queue
--
Reset and Queue Experiment

Thoughts?

Related: exp run is doing too much, and at least modifying params could be separated from it.

For non-queue experiments, modifying params in the CLI is a convenience to avoid opening an editor, which isn't necessary for VS Code. You could have a separate option to modify params without doing anything else (either using the existing interface or by opening the params files in the editor). This may simplify the menu options and make it easier to review changes before running experiments.

For queuing, you could:

Keep the existing options.
Have a sub-menu/prompt after selecting "Queue Experiment" for whether to modify params.
Enforce a workflow where users must make any modifications in the workspace and then queue.

shcheklein · 2022-05-17T20:29:45Z

Makes sense @dberenbaum !

Do we need to / if we can detect that resume / reset are needed? E.g. if we see from the workspace that no matter what you do it will be the new experiment so that we can simplify the actions? And if it's

mattseddon · 2022-05-18T00:58:48Z

For the menus, I would suggest:

Resume Experiment

Reset and Run Experiment

Run All Queued Experiments [should this be in a separate section below since it's about all queued experiments?]

Modify Experiment Param(s) and Resume

Modify Experiment Param(s), Reset, and Run

Modify Experiment Param(s), Reset, and Queue

Reset and Queue Experiment

Thoughts?

[Q] Can a non-checkpoint experiment be resumed? Does it make sense to even show these options when a project does not have checkpoints? Under what circumstances should an experiment be resumed?

For non-queue experiments, modifying params in the CLI is a convenience to avoid opening an editor, which isn't necessary for VS Code. You could have a separate option to modify params without doing anything else (either using the existing interface or by opening the params files in the editor). This may simplify the menu options and make it easier to review changes before running experiments.

I think we should cover this as part of #1537 rather than completely change the workflow now. Opening the file could be an interim step towards closing that story.

Next steps for me:

Merge this PR.
Raise a follow up based on further discussion here. Likely change run to resume, etc. Just need an answer on how relevant those menus are for non-checkpoint based projects.

codeclimate · 2022-05-18T01:04:06Z

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

This pull request will bring the total coverage in the repository to 96.6% (-0.1% change).

View more on Code Climate.

daavoo · 2022-05-18T10:38:11Z

[Q] Can a non-checkpoint experiment be resumed?

Afaik, no. DVC will delete the outputs of the stage.

Does it make sense to even show these options when a project does not have checkpoints?

IMO, no. There is no concept (at least in DVC context) of resetting or resuming without checkpoints. Just run.

dberenbaum · 2022-05-18T17:00:38Z

extension/package.nls.json

@@ -30,6 +30,7 @@
  "command.pushTarget": "Push",
  "command.modifyExperimentParamsAndQueue": "Modify Experiment Param(s) and Queue",
  "command.modifyExperimentParamsAndRun": "Modify Experiment Param(s) and Run",
+  "command.modifyExperimentParamsResetAndRun": "Modify Experiment Param(s) Reset and Run",


Suggested change

"command.modifyExperimentParamsResetAndRun": "Modify Experiment Param(s) Reset and Run",

"command.modifyExperimentParamsResetAndRun": "Modify Experiment Param(s), Reset and Run",

dberenbaum · 2022-05-18T17:00:58Z

extension/package.nls.json

@@ -60,6 +61,7 @@
  "command.views.experimentsTree.queueExperiment": "Modify Param(s) and Queue",
  "command.views.experimentsTree.removeExperiment": "Remove",
  "command.views.experimentsTree.runExperiment": "Modify Param(s) and Run",
+  "command.views.experimentsTree.resetRunExperiment": "Modify Param(s) Reset and Run",


Suggested change

"command.views.experimentsTree.resetRunExperiment": "Modify Param(s) Reset and Run",

"command.views.experimentsTree.resetRunExperiment": "Modify Param(s), Reset and Run",

dberenbaum · 2022-05-18T17:04:22Z

Yes, I thought these options were only shown if existing checkpoints are found. If not, resume/reset options will not make sense.

shcheklein · 2022-05-18T17:21:32Z

@dberenbaum @mattseddon @daavoo can we see if there are exiting checkpoints?

dberenbaum · 2022-05-18T20:39:33Z

Yes, checkpoint commits should include a checkpoint_tip field, which I think are already being used for visual cues to show checkpoints as collapsible dotted lines under each experiment. I think it only makes sense to show reset/resume options when these exist.

mattseddon · 2022-05-19T00:49:13Z

I'll follow up and tag everyone in the new PR.

dberenbaum · 2022-05-20T17:56:55Z

Run All Queued Experiments

Follow-up on this language. Given upcoming changes in iterative/dvc#7592, it might make sense to reword to "Start the [Experiments] Queue."

mattseddon added the product PR that affects product label May 16, 2022

mattseddon self-assigned this May 16, 2022

mattseddon added 4 commits May 17, 2022 10:15

add run reset experiment everywhere

cf77eca

hoist remove in context menu

355c7bc

reorder context menu

7f9314c

always have run reset in the context menu

83eccac

mattseddon force-pushed the add-run-reset-everywhere branch from 9fac1f4 to 83eccac Compare May 17, 2022 01:11

mattseddon marked this pull request as ready for review May 17, 2022 01:15

mattseddon requested review from shcheklein and a team May 17, 2022 01:18

shcheklein approved these changes May 17, 2022

View reviewed changes

sroy3 approved these changes May 17, 2022

View reviewed changes

Merge branch 'main' into add-run-reset-everywhere

4970571

mattseddon enabled auto-merge (squash) May 18, 2022 00:59

mattseddon merged commit 17cab9a into main May 18, 2022

mattseddon deleted the add-run-reset-everywhere branch May 18, 2022 01:05

dberenbaum reviewed May 18, 2022

View reviewed changes

This was referenced May 19, 2022

Rework experiment commands and context menus to vary based on whether experiments have checkpoints #1738

Merged

Add experiment icons to editor/title when params file is open #1740

Merged

mattseddon mentioned this pull request May 21, 2022

Improve user facing text for dvc.runQueuedExperiments #1751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experiment run reset to menus #1719

Add experiment run reset to menus #1719

mattseddon commented May 16, 2022 •

edited

Loading

shcheklein left a comment

mattseddon commented May 17, 2022

dberenbaum commented May 17, 2022

shcheklein commented May 17, 2022

mattseddon commented May 18, 2022

Run All Queued Experiments [should this be in a separate section below since it's about all queued experiments?]

Modify Experiment Param(s), Reset, and Queue

codeclimate bot commented May 18, 2022

daavoo commented May 18, 2022

dberenbaum May 18, 2022

dberenbaum May 18, 2022 •

edited

Loading

dberenbaum commented May 18, 2022

shcheklein commented May 18, 2022

dberenbaum commented May 18, 2022

mattseddon commented May 19, 2022

dberenbaum commented May 20, 2022

	"command.modifyExperimentParamsResetAndRun": "Modify Experiment Param(s) Reset and Run",
	"command.modifyExperimentParamsResetAndRun": "Modify Experiment Param(s), Reset and Run",

	"command.views.experimentsTree.resetRunExperiment": "Modify Param(s) Reset and Run",
	"command.views.experimentsTree.resetRunExperiment": "Modify Param(s), Reset and Run",

Add experiment run reset to menus #1719

Add experiment run reset to menus #1719

Conversation

mattseddon commented May 16, 2022 • edited Loading

Screenshots

shcheklein left a comment

Choose a reason for hiding this comment

mattseddon commented May 17, 2022

dberenbaum commented May 17, 2022

shcheklein commented May 17, 2022

mattseddon commented May 18, 2022

Run All Queued Experiments [should this be in a separate section below since it's about all queued experiments?]

Modify Experiment Param(s), Reset, and Queue

codeclimate bot commented May 18, 2022

daavoo commented May 18, 2022

dberenbaum May 18, 2022

Choose a reason for hiding this comment

dberenbaum May 18, 2022 • edited Loading

Choose a reason for hiding this comment

dberenbaum commented May 18, 2022

shcheklein commented May 18, 2022

dberenbaum commented May 18, 2022

mattseddon commented May 19, 2022

dberenbaum commented May 20, 2022

mattseddon commented May 16, 2022 •

edited

Loading

dberenbaum May 18, 2022 •

edited

Loading