Add 'dvc exp run' command #22

ryanraposo · 2020-12-15T22:38:55Z

Can be accessed via VS Code's command palette (Ctrl+Shift+P) or the extension's command view
Can be bound to keyboard shortcuts in expected ways
Uses a labeled integrated terminal to run the command in context automatically

rogermparent · 2020-12-16T01:00:11Z

Works on my machine! Some thoughts:

We'll certainly have to expand on this in the future with a way to recognize when the experiment is finished running and send a message to Webviews accordingly. Depending on limitations of the VS Code terminal, this may warrant using child_process like we do in DvcReader.ts
There are a few more options to consider when invoking an experiment run, like queuing and running experiments with dvc exp run --queue and dvc exp run --run-all, respectively. There may be enough alternate options to warrant something like a "Run Experiment" submenu.

That said, even just the change of adding this button is worth merging, leaving iterations like the points I mentioned to their own Issues/PRs.

rogermparent · 2020-12-16T01:02:27Z

extension/src/extension.ts

@@ -91,6 +91,14 @@ export class Extension {
      })
    )

+    this.dispose.track(
+      commands.registerCommand('dvc-integration.runExperiment', () => {
+        const terminal = window.createTerminal('DVC')


If I understand correctly, the "rise or run" behavior I mentioned before would probably hook into the extension here and keep track of terminal across invocations. I hope to do this kind of technique for Webview invocations as well.

I think I actually removed the context of this comment before posting, but by "rise or run" I essentially mean that we may want to use a single Terminal for DVC operations, only spawning a new one when the original is closed.

Yep, we can also check window.terminals which is an array of existing terminals to achieve simple get/create behaviour.

ryanraposo · 2020-12-16T01:23:06Z

We'll certainly have to expand on this in the future with a way to recognize when the experiment is finished running and send a message to Webviews accordingly. Depending on limitations of the VS Code terminal, this may warrant using child_process like we do in DvcReader.ts

Agreed!

There are a few more options to consider when invoking an experiment run, like queuing and running experiments with dvc exp run --queue and dvc exp run --run-all, respectively. There may be enough alternate options to warrant something like a "Run Experiment" submenu.

That's a great insight & idea.

That said, even just the change of adding this button is worth merging, leaving iterations like the points I mentioned to their own Issues/PRs.

That was my thinking also.

Thanks for an excellent review and all your help generally @rogermparent, really has been a pleasure so far. 🙂

rogermparent · 2020-12-16T01:31:01Z

@ryanraposo Do you want to do any touch-ups? I'm not sure if you have merge permissions, but if not I can merge this in right now.

ryanraposo · 2020-12-16T01:43:34Z

@ryanraposo Do you want to do any touch-ups? I'm not sure if you have merge permissions, but if not I can merge this in right now.

I can merge it, and we'll stick with leaving improvements to future PRs like discussed :)

shcheklein · 2020-12-16T01:46:46Z

extension/src/extension.ts

+    this.dispose.track(
+      commands.registerCommand('dvc-integration.runExperiment', () => {
+        const terminal = window.createTerminal('DVC')
+        terminal.sendText('dvc exp run')


quick question - what is the working directory for this? what will be the mechanism to define this?

cwd is an optional parameter for createTerminal, and if it isn't defined the current workspace (opened folder) is used for the terminal, with the same being true for general VS Code behavior when it comes to integrated terminals

okay, the question - is that what users would expect for this action?

@shcheklein my thoughts on improving expected integration:

Since VS Code git integration is analogous and built-in, it can be argued that it should serve as a model for expected behavior. Git commands (via command palette or UI) use Ouput (read-only) channels, so they are more appropriate here.

Commands with labels in a sidebar (Activity Bar) tree view like we have is unexpected. IMO this is partly because of the additional access VS Code contributes for all commands automatically.

Contextual usage (right click) would be useful/expected for this command. We should add it to the Explorer view and to editors with relevant files open, and use a 'WHEN' condition to limit its appearance to file type; with exceptions or failures being handled/indicated in Output.

If we want additional access to a command like this in our sidebar it should be supplemental to features that justify the sidebar, as they don't themselves.

Given a justified sidebar, comparable instances would lead a user to expect them here, as 'view/title' commands:

Note also the Output channel with output from git, bottom right. These channels are typically per-extension/source and though it isn't clear from the screenshot, they are labelled (hover) and can be cycled/toggled via dropdown like terminals.

In normal use, they are feeds that don't show themselves even when actions cause output. Tasks in VS Code use a 'terminal' with unique behaviour (also read only), show when triggered, and are re-used by only that task specifically.

One certainty (given this approach) is that Tasks shouldn't be emulated too closely, as our commands aren't Tasks in VS Code terms.

An important note (for @rogermparent also): the VS Code API gives us a number of tools for event monitoring. They're generally underutilized and I can already see them being the foundation for some complex solves in this project.

An example: OnDidChange event listener for particular documents--our webviews can be very dynamic with simple code if we leverage these tools in cases where updates can be inferred from changes to a file.

An important note (for @rogermparent also): the VS Code API gives us a number of tools for event monitoring. They're generally underutilized and I can already see them being the foundation for some complex solves in this project.

An example: OnDidChange event listener for particular documents--our webviews can be very dynamic with simple code if we leverage these tools in cases where updates can be inferred from changes to a file.

That is incredibly useful to know. Monitoring files like params.yaml and/or dvc.yaml will likely be absolutely core in this project. I'm open to simplifying the webviews too whenever possible- they'll need some state for things like sorting, but ideally as little as possible if I understand correctly.

@iterative/engineering Are there any files other than dvc.yaml and params.yaml that would use useful to watch for changes? I can see this feature being used for user config, but there may also be some files where watching will be useful for monitoring when DVC actions happen outside of the plugin- maybe dvc.lock?.

Yeah! I was hoping it'd take some stress off. Was mistaken about the 'OnDidChange' format. Here's the more accurate snippet:

const fileWatcher = vscode.workspace.createFileSystemWatcher(pattern); fileWatcher.onDidChange(() => doThing());

@rogermparent as we come to displaying metrics (the bottom part) - they might come from us tracking changes to a directory.

clearly all changes to the workspace (e.g. even to detect that it's dirty).

Changes to dvc.lock, .dvc files that correspond to data and data itself potentially to update it's status in the files tree (when we get there).

It might be tricky though, since we'll need to do find and track changes to files recursively.

ryanraposo added 3 commits December 15, 2020 12:30

Add preliminary 'exp run' command

de59727

Add command to Command View

cbd5025

💄

749e779

ryanraposo marked this pull request as ready for review December 16, 2020 00:24

Add 'Run Experiment' command to documentation

d447495

rogermparent self-requested a review December 16, 2020 00:38

rogermparent approved these changes Dec 16, 2020

View reviewed changes

ryanraposo merged commit 05c2b7f into master Dec 16, 2020

shcheklein reviewed Dec 16, 2020

View reviewed changes

ryanraposo mentioned this pull request Dec 16, 2020

Anticipate expectations for dvc commands #26

Closed

maxagin mentioned this pull request Jun 23, 2022

Hide toggle selection icon for empty lists #1564

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'dvc exp run' command #22

Add 'dvc exp run' command #22

ryanraposo commented Dec 15, 2020 •

edited

Loading

rogermparent commented Dec 16, 2020

rogermparent Dec 16, 2020

rogermparent Dec 16, 2020

ryanraposo Dec 16, 2020

ryanraposo commented Dec 16, 2020 •

edited

Loading

rogermparent commented Dec 16, 2020

ryanraposo commented Dec 16, 2020

shcheklein Dec 16, 2020

ryanraposo Dec 16, 2020

shcheklein Dec 16, 2020

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

rogermparent Dec 17, 2020

ryanraposo Dec 17, 2020

shcheklein Dec 18, 2020

Add 'dvc exp run' command #22

Add 'dvc exp run' command #22

Conversation

ryanraposo commented Dec 15, 2020 • edited Loading

rogermparent commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanraposo commented Dec 16, 2020 • edited Loading

rogermparent commented Dec 16, 2020

ryanraposo commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanraposo Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

ryanraposo Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

ryanraposo Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

ryanraposo Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanraposo commented Dec 15, 2020 •

edited

Loading

ryanraposo commented Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading

ryanraposo Dec 16, 2020 •

edited

Loading