Skip to content

exp remove: Update help for --all flag#3216

Merged
iesahin merged 2 commits into
masterfrom
daavoo-patch-1
Feb 1, 2022
Merged

exp remove: Update help for --all flag#3216
iesahin merged 2 commits into
masterfrom
daavoo-patch-1

Conversation

@daavoo
Copy link
Copy Markdown
Contributor

@daavoo daavoo commented Jan 26, 2022

It is not true that --all includes --queue. Options need to be combined in order to effectively remove all ran and queued experiments

It is not true that `--all` includes `--queue`. Options need to be combined in order to effectively remove all runned and queued experiments
@daavoo daavoo requested a review from a team January 26, 2022 13:49
@shcheklein shcheklein temporarily deployed to dvc-org-daavoo-patch-1-dhdxi5t January 26, 2022 13:49 Inactive
@daavoo daavoo self-assigned this Jan 26, 2022
@daavoo daavoo added the C: ref Content of /doc/*-reference label Jan 26, 2022
Comment thread content/docs/command-reference/exp/remove.md
Comment thread content/docs/command-reference/exp/remove.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-daavoo-patch-1-dhdxi5t January 27, 2022 08:07 Inactive
Comment on lines -29 to +30
- `-A`, `--all` - remove all experiments (includes `--queue`).
- `-A`, `--all` - remove all experiments that have been run. Use `--queue` to
remove queued ones.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this apply as well for exp push/list/pull ? Cc @dberenbaum and @karajan1001 (per #3191-3)

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jan 27, 2022

Choose a reason for hiding this comment

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

And what about exp show -a/A? Should we mention that in that case queued experiments ARE included in the output? May be a product question on whether the behavior is consistent, not sure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this apply as well for exp push/list/pull ? Cc @dberenbaum and @karajan1001 (per #3191-3)

Currently push/list/pull does not work on queued ones.

And what about exp show -a/A? Should we mention that in that case queued experiments ARE included in the output? May be a product question on whether the behavior is consistent, not sure.

This is of course a problem. Cc @pmrowla .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, they are currently excluded from everything except for exp run --queue, and exp remove --queue, and exp show. For now, I think that behavior is fine if correctly documented.

Eventually, queue operations should be in a separate command. See treeverse/dvc#7275 (comment) and treeverse/dvc#5615.

Copy link
Copy Markdown
Contributor Author

@daavoo daavoo Jan 31, 2022

Choose a reason for hiding this comment

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

And what about exp show -a/A? Should we mention that in that case queued experiments ARE included in the output?

To clarify, queued experiments will be shown even without -a/A flag.

@daavoo
Copy link
Copy Markdown
Contributor Author

daavoo commented Jan 31, 2022

@jorgeorpinel Should we merge this and extract (if needed) #3216 (comment) ?

Copy link
Copy Markdown
Contributor

@iesahin iesahin left a comment

Choose a reason for hiding this comment

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

Thank you @daavoo

@iesahin iesahin merged commit 5a15db4 into master Feb 1, 2022
@iesahin iesahin deleted the daavoo-patch-1 branch February 1, 2022 09:36
iesahin pushed a commit that referenced this pull request Apr 11, 2022
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C: ref Content of /doc/*-reference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants