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

Queue: make sure tasks are hidden and all actions make sense in the table #3091

Closed
5 tasks done
shcheklein opened this issue Jan 11, 2023 · 10 comments
Closed
5 tasks done
Assignees
Labels
A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

shcheklein commented Jan 11, 2023

As we discussed during the planning, I don't think exposing tasks in the table, quick pick makes sense. E.g. remove action - we should be (at least first) be able to remove any experiment including queued, failed in the queue, etc, etc.

From the user perspective- they don't care about tasks, celery, etc ... how it's implemented etc. All that stuff should be hidden by the experiments table and actions on experiments.

  • Make sure that we show all experiments in the table
  • Make sure that same actions can be applied seamlessly to all of those experiments. E.g. I can pick a failed and successful experiment + a queued experiments and delete them (do we need DVC support for this)
  • DVC should be providing the same experience I think (cc @dberenbaum )
  • Review quick picks, etc - don't expose extra actions on the queue for now (e.g. we don't want to have two removes, etc).
  • Discuss why would we actually want to expose this information as an advanced scenario, etc and do a followup after that
@shcheklein shcheklein added A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer priority-p1 Regular product backlog labels Jan 11, 2023
@dberenbaum
Copy link
Contributor

LGTM!

  • Make sure that same actions can be applied seamlessly to all of those experiments. E.g. I can pick a failed and successful experiment + a queued experiments and delete them (do we need DVC support for this)

VS Code should only need to pass the experiment names to dvc exp remove and it should work:

$ dvc queue status
Task     Name        Created       Status
be372c8  taboo-trey  08:47 AM      Queued
f285967  sworn-cuss  08:47 AM      Failed
63ea1ab  regal-weft  Jan 06, 2023  Success

Worker status: 0 active, 0 idle

$ dvc exp remove taboo-trey sworn-cuss regal-weft
Removed experiments: taboo-trey,sworn-cuss,regal-weft

@mattseddon
Copy link
Member

[Q] Do we want to hide dvc queue kill <exp>? Seems like we would want to provide two options with dvc queue stop:

  1. Stop the current queue workers and leave the experiments currently being processed running.
  2. Kill anything that is running and stop the queue workers.

@shcheklein
Copy link
Member Author

@mattseddon are those two different questions? or one? I'm trying to better understand it, sorry .. :)

@mattseddon
Copy link
Member

Things that are currently implemented

  1. queue start (start queue workers)
  2. queue stop (stop queue workers -> leaves any currently running queue tasks to complete)
  3. queue kill (stop currently processing queue tasks, i.e experiments running in the queue - user is given all executing queue tasks as options)
  4. remove queue tasks (remove groups of queue tasks, e.g success, failed, queued or all)

From OP: 4 will be removed.

Questions

  1. should dvc queue stop --kill be added as an option to stop queue workers and any currently running queue tasks?
  2. should 3 be removed or can it be left and renamed to dvc.killExperimentsRunningInQueue or dvc. stopExperimentsRunningInQueue?
  3. should 3 be an all-or-nothing option to stop all currently executing queue tasks?

@shcheklein
Copy link
Member Author

My take:

  • queue stop name is not clear. We should come up with an explicit name for this - Experiments: complete running and stop (or pause?)?
  • Queue start - Experiments: run queued
  • should 3 be an all-or-nothing option to stop all currently executing queue tasks? - I think yes, we should have "Experiments: stop all" and "Experiments: stop" (not only tasks, it means all running experiments - queued, temp dir, workspace). Later we can differentiate if there is a need. How is it different from delete / drop / remove though?
  • Means 1 is not needed, 3 - yes, can be removed (or replaced)
  • stop / remove / etc actions - there should be a way to select multiple exps and apply (via table and via quick pick (?))

Overall, it seems intimidating, even semantics of this.

@mattseddon
Copy link
Member

My take:

  • queue stop name is not clear. We should come up with an explicit name for this - Experiments: complete running and stop (or pause?)?
  • Queue start - Experiments: run queued

User facing names are currently Stop the Experiments Queue & Start the Experiments Queue. Do we really want to try and hide the concept of the queue/queue workers?

  • should 3 be an all-or-nothing option to stop all currently executing queue tasks? - I think yes, we should have "Experiments: stop all" and "Experiments: stop" (not only tasks, it means all running experiments - queued, temp dir, workspace). Later we can differentiate if there is a need. How is it different from delete / drop / remove though?

There is currently no way to stop an experiment that is being run inside the workspace outside of the VS Code context (i.e user runs it through the terminal). There is also currently no link between the ID of the experiment running in the workspace and the process that we can stop to stop that experiment, we could try to proxy the process if there is 1 running experiment and 1 process but that could go horribly wrong. This will make mashing these two concepts together problematic.

  • stop / remove / etc actions - there should be a way to select multiple exps and apply (via table and via quick pick (?))

Remove action is implemented in both the table and a quick pick. The quick pick is about to be updated here: #3093. For stop we have a quick pick under the kill name for experiments running in/from the queue. We could add this functionality to the table. For previously stated reasons this will be more difficult for experiments not running in the queue.

Overall, it seems intimidating, even semantics of this.

I personally don't think that the concept the DVC team has put forwards/implemented for the queue is overly complicated or different from what I've seen elsewhere. I don't think we need to diverge too far from the semantics. I don't think we should go much further than integrating kill into the table and tweaking the name slightly to reflect stopping the experiment.

@shcheklein
Copy link
Member Author

User facing names are currently Stop the Experiments Queue & Start the Experiments Queue. Do we really want to try and hide the concept of the queue/queue workers?

My main concern that atm it requires an explanation Stop the Experiments Queue is not self descriptive (I can't understand the semantics of it w/o reading the docs). It would be fine if we can add a description for example. Start the queue is better, but would it be even better to say "Run queued experiments" ... or Just have run all button / action?

I'm not trying to hide the queue as a term. Since we are queueing experiments it's fine that we use it in some ways and forms. What I'm trying to think about and optimize is that users immediately understand what each action does, they are not overwhelmed by the number of actions, etc.

There is currently no way to stop an experiment that is being run inside the workspace outside of the VS Code context (i.e user runs it through the terminal). There is also currently no link between the ID of the experiment running in the workspace and the process that we can stop to stop that experiment, we could try to proxy the process if there is 1 running experiment and 1 process but that could go horribly wrong. This will make mashing these two concepts together problematic.

Means that stop button doesn't work for these cases at all, even though we can show that experiment in the table? I thought that we had pid from DVC (and probably we can add it if needed)?

Anyways, even there are some limitations, we should strive for semantic generalization and simplicity I think.

@mattseddon
Copy link
Member

There is currently no way to stop an experiment that is being run inside the workspace outside of the VS Code context (i.e user runs it through the terminal). There is also currently no link between the ID of the experiment running in the workspace and the process that we can stop to stop that experiment, we could try to proxy the process if there is 1 running experiment and 1 process but that could go horribly wrong. This will make mashing these two concepts together problematic.

Means that stop button doesn't work for these cases at all, even though we can show that experiment in the table? I thought that we had pid from DVC (and probably we can add it if needed)?

Yes, the stop button will not work under those circumstances. That is why I started hiding it for these cases in #3027. Whilst I was working on that change I tried to find a place where the PID for the process running experiments is exposed by DVC but I couldn't find one.

@mattseddon
Copy link
Member

Make sure that same actions can be applied seamlessly to all of those experiments. E.g. I can pick a failed and successful experiment + a queued experiments and delete them (do we need DVC support for this)

Seems to work although the CLI does throw an error under the circumstances listed in this comment.

@mattseddon
Copy link
Member

Should be ready for the next round of review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer priority-p1 Regular product backlog
Projects
None yet
Development

No branches or pull requests

3 participants