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

exp show: display running/queued state for experiments #6174

Merged
merged 14 commits into from
Jul 12, 2021

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Jun 15, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Will close #5965

  • For the DVC UI, this PR adds State column that will currently be Running, Queued or empty
    • If the table has no running or queued experiments the column will not be displayed
    • If State is Running, an additional Executor column will be displayed noting where the experiment is being run, currently this is limited to local (workspace) or local (background)
  • For --show-json (VSCode), experiment dicts now have a running key that will be true or false, and an executor key which will be an optional string executor name
  • Fixes bug where main repo would be locked during --temp or --queue/--run-all runs (related to exp show: Allow running while DVC is lockedΒ #5739) which prevented exp show from being usable

Regular experiments example:
asciicast

Checkpoints example:
asciicast

@pmrowla pmrowla self-assigned this Jun 15, 2021
@pmrowla pmrowla added A: experiments Related to dvc exp enhancement Enhances DVC ui user interface / interaction labels Jun 15, 2021
@pmrowla pmrowla requested a review from dberenbaum June 15, 2021 11:30
@pmrowla pmrowla changed the title [WIP] exp show: display running/queued state for experiments exp show: display running/queued state for experiments Jun 15, 2021
@pmrowla
Copy link
Contributor Author

pmrowla commented Jun 15, 2021

Regarding docs, exp show usage examples need to be updated throughout the docs. I'm planning on adding a PR to display user-specified names for queued exps and will do a single docs PR w/the updated examples after that

@pmrowla pmrowla marked this pull request as ready for review June 15, 2021 11:31
Comment on lines 153 to 156
if exp.get("running"):
state = "Run"
elif exp.get("queued"):
state = "Queue"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be Running/Queued? Just feels a bit odd. Maybe this has been discussed somewhere. Just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running/Queued makes more sense but I'm not really sure what's best here given that table width is a concern

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that Running/Queued is better, unless someone can come up with something shorter that's still clear. We could include a --no-status option to keep it narrower. Experiment and Created take up a lot of space, so we could also look into ways to narrow those if needed.

Copy link
Collaborator

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

It looks good to me for non-checkpoint experiments.

For checkpoints experiments, it's a little confusing because:

  1. The Run status shows next to the already completed checkpoint. Maybe we should be showing the experiment name on a separate line from individual checkpoints? That would also narrow the first column.
  2. For workspace runs, the status is Run for both the workspace row and the experiment row (which only exists after the first checkpoint). So it can look like there are two experiments running, one of which got added after the first checkpoint. Is there a way to indicate that the workspace is the same as one of the experiments, at least while the experiment is running?

Comment on lines 153 to 156
if exp.get("running"):
state = "Run"
elif exp.get("queued"):
state = "Queue"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that Running/Queued is better, unless someone can come up with something shorter that's still clear. We could include a --no-status option to keep it narrower. Experiment and Created take up a lot of space, so we could also look into ways to narrow those if needed.

@pmrowla
Copy link
Contributor Author

pmrowla commented Jun 17, 2021

@dberenbaum

  1. The Run status shows next to the already completed checkpoint.

This makes sense to me because when you are resuming/continuing a checkpoint run, you are starting from the last completed checkpoint (rather than starting from the queued row that only has params and no metrics/outputs)

Maybe we should be showing the experiment name on a separate line from individual checkpoints? That would also narrow the first column.

What would we do here for regular experiments? When #6050 is done we will have names in the rows for these queued/running experiments, and will have the same issue. I don't think it makes sense to take up 2 rows for a single experiment.

  1. For workspace runs, the status is Run for both the workspace row and the experiment row (which only exists after the first checkpoint). So it can look like there are two experiments running, one of which got added after the first checkpoint. Is there a way to indicate that the workspace is the same as one of the experiments, at least while the experiment is running?

It seems like what we are going to eventually need is to display information about where an experiment is running (for remote executors) but I'm not sure where to indicate that right now. We could omit Run entirely from the workspace row, and display the state for experiment rows as something like Running (workspace)/ Running (background) but this is going to keep making the table wider

@dberenbaum
Copy link
Collaborator

This makes sense to me because when you are resuming/continuing a checkpoint run, you are starting from the last completed checkpoint (rather than starting from the queued row that only has params and no metrics/outputs)

Extracted to #6194.

It seems like what we are going to eventually need is to display information about where an experiment is running (for remote executors) but I'm not sure where to indicate that right now. We could omit Run entirely from the workspace row, and display the state for experiment rows as something like Running (workspace)/ Running (background) but this is going to keep making the table wider

Yes, we need a column like executor or location or something. Why not add it now? Maybe we can autohide it and the state column when it's empty (nothing is running)?

@@ -304,7 +309,7 @@ def experiments_table(

from dvc.compare import TabularData

headers = ["Experiment", "rev", "queued", "typ", "Created", "parent"]
headers = ["Experiment", "rev", "typ", "Created", "parent", "State"]
Copy link
Member

@skshetry skshetry Jun 18, 2021

Choose a reason for hiding this comment

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

I am a bit confused about what it should be: State or Status. Both seem to make sense to me. Is State most appropriate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think either term works here. I just used state because status has some other connotations in DVC

@pmrowla pmrowla requested a review from a team as a code owner June 29, 2021 06:29
@pmrowla pmrowla requested a review from dberenbaum June 29, 2021 06:51
@pmrowla
Copy link
Contributor Author

pmrowla commented Jun 29, 2021

@dberenbaum I've added a separate executor column that will display where the experiment is running, and both state and executor are only shown if there is actually any running/queued experiments in the table. (see the 2 updated screen recordings in the top post)

@skshetry
Copy link
Member

I think that we are mixing two things in the exp show - list and metrics/params show (which is collapsed in non-pager mode). Would be great to revisit the table formats for 3.0 (especially considering remote executors might add more columns here). cc @dberenbaum

@dberenbaum
Copy link
Collaborator

I think that we are mixing two things in the exp show - list and metrics/params show (which is collapsed in non-pager mode). Would be great to revisit the table formats for 3.0 (especially considering remote executors might add more columns here). cc @dberenbaum

Do you mean that columns like status and executor should be shown in a separate command and not in dvc exp show?

@dberenbaum
Copy link
Collaborator

@pmrowla Looks great!

One very minor comment: can we shorten local (workspace) and local (background) text to just workspace and background or temp (since that's the flag name) or similar? local seems redundant now, and even when we have remote executors, we can specify them by name.

@@ -381,16 +395,22 @@ def show_experiments(
if no_timestamp:
td.drop("Created")

for col in ("State", "Executor"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for other columns to be empty? If so, would we want to hide them, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There shouldn't be any other columns in the table right now that can be empty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Related to dvc exp enhancement Enhances DVC ui user interface / interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exp show: include running experiment
4 participants