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

[mattseddon]

1/2 main <- this <- #1739

Follow up based on the feedback provided in #1719. Thanks, @dberenbaum, @daavoo, @shcheklein.

This PR reworks the extension UI for running experiments. The experiments table webview context menus will be updated in the next PR.

As the title states, we will show different sets of commands based on whether or not the workspace contains checkpoint experiments.

No Checkpoints

The simpler version is when there are no checkpoints => There is a single run command. Versions of this are shown throughout the UI.

Checkpoints

When there are checkpoints => There are two commands, "resume" and "reset and run". Versions of these commands are shown throughout the UI

Please LMK if you have any feedback.

Note: We decide if a project is using checkpoint based experiments by parsing the dvc.yaml(s) in the workspace.

Relates to #1711

[mattseddon]

[F] This duplication is ugly and cumbersome but I could not find another way around.

[daavoo]

@mattseddon Would it make sense to make the choice between reset or resume a "submenu" of Run experiment?

Such as, if you have checkpoints, you still see the same comands/tooltips, but when you chose Run experiment an additional menu to select whether to reset or resume pops?

[mattseddon]

@mattseddon Would it make sense to make the choice between reset or resume a "submenu" of Run experiment?

Such as, if you have checkpoints, you still see the same comands/tooltips, but when you chose Run experiment an additional menu to select whether to reset or resume pops?

@daavoo fair suggestion. We could potentially put "conditional submenus" for context menus in that look like this:

The further we go down this rabbit hole the worse our package.json gets the file is already at 2651 lines and we can only indirectly test the logic.

For everything else, the option would be to either:

1. Add an alt command where the command is changed if the user presses alt + clicks on icon (see here for more details)
2. Send the extra step to the quick pick

I actually removed one example of 1 in this PR because myself and @shcheklein were very confused by the behaviour of the button when reviewing the product one day.

The extra step in 2 would definitely annoy me.

[dberenbaum]

Note: We decide if a project is using checkpoint based experiments by parsing the dvc.yaml(s) in the workspace.

This is probably simplest and best, but there are some potential downsides to consider:

1. If no checkpoints actually exist yet, there's not really a "resume" or "reset" possible.
2. If there are multiple pipelines or dvc.yaml files, I'm not sure how to decide when to show the checkpoints menu, but it's unclear whether the extension is intended to address this use case in general (and TBH it's not that well supported in dvc exp either).

[daavoo]

If no checkpoints actually exist yet, there's not really a "resume" or "reset" possible.

Could check dvc.lock instead?

[mattseddon]

@dberenbaum

If no checkpoints actually exist yet, there's not really a "resume" or "reset" possible.

[Q] This should only happen before any experiment has been committed, right?

2. If there are multiple pipelines or dvc.yaml files, I'm not sure how to decide when to show the checkpoints menu, but it's unclear whether the extension is intended to address this use case in general (and TBH it's not that well supported in dvc exp either).

Currently if there is any one dvc.yaml with checkpoints in the workspace we'll always show resume/reset and run. I think it would be ok to revisit when/if we get a feature request to change the logic. Wdyt?

@daavoo

If no checkpoints actually exist yet, there's not really a "resume" or "reset" possible.

Could check dvc.lock instead?

What would I be looking for? There is nothing obvious in the dvc.lock in the demo project.

[dberenbaum]

[Q] This should only happen before any experiment has been committed, right?

Correct AFAIK.

Currently if there is any one dvc.yaml with checkpoints in the workspace we'll always show resume/reset and run. I think it would be ok to revisit when/if we get a feature request to change the logic. Wdyt?

Yup, sounds good.

[shcheklein]

First of all, thanks, Matt for quickly changing this! It can improve the experience a lot 🙏

Currently if there is any one dvc.yaml with checkpoints in the workspace we'll always show resume/reset and run. I think it would be ok to revisit when/if we get a feature request to change the logic. Wdyt?

dvc.yaml can be located anywhere, and it can one or many of those. So, it's quite a strong assumption ...

We decide if a project is using checkpoint based experiments by parsing the dvc.yaml(s) in the workspace.

what was the reason / any details on this?

[mattseddon]

what was the reason / any details on this?

We previously added this functionality to determine whether a project has checkpoints in #1281 to fix a race condition with workspace vs running experiment plots.

We do this by watching this glob: join(this.dvcRoot, '**', 'dvc.yaml'), reading any yaml that has been updated and checking to see if any of the outs have a checkpoint field. We also initialize the workspace by first finding all of the dvc.yamls in the workspace and running them all through the same process.

I reused the functionality because it was already there. If we simply relied on the exp show data then when there are no experiments in the workspace we would only offer the option to run aka resume. IMO this would be a bigger issue.

[shcheklein]

We also initialize the workspace by first finding all of the dvc.yamls in the workspace and running them all through the same process.

This is a pretty heavy fix I would say. Since we already have it that's fine. But I doubt it's sustainable. Scanning the whole repo for dvc.yaml can be expensive. Do we at least ignore DVC-tracked directories (they should be in .gitignore)?

[mattseddon]

We also initialize the workspace by first finding all of the dvc.yamls in the workspace and running them all through the same process.

This is a pretty heavy fix I would say. Since we already have it that's fine. But I doubt it's sustainable. Scanning the whole repo for dvc.yaml can be expensive. Do we at least ignore DVC-tracked directories (they should be in .gitignore)?

All of the watching/find is using tools provided by the vscode API. The team at MS has spent a lot of time optimising both of them. It shouldn't turn into an issue.

[codeclimate]

Code Climate has analyzed commit 03ed320 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2

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

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

View more on Code Climate.