Skip to content

exp show: add --rev option#3187

Merged
jorgeorpinel merged 1 commit into
treeverse:masterfrom
karajan1001:add_rev_to_exp_show
Mar 10, 2022
Merged

exp show: add --rev option#3187
jorgeorpinel merged 1 commit into
treeverse:masterfrom
karajan1001:add_rev_to_exp_show

Conversation

@karajan1001
Copy link
Copy Markdown
Contributor

@karajan1001 karajan1001 commented Jan 17, 2022

@karajan1001 karajan1001 marked this pull request as draft January 17, 2022 11:36
@karajan1001 karajan1001 marked this pull request as ready for review January 18, 2022 08:10
Comment thread content/docs/command-reference/exp/show.md Outdated
@iesahin iesahin added C: ref Content of /doc/*-reference ⌛ status: wait-core-merge Waiting for related product PR merge/release labels Jan 19, 2022
@shcheklein shcheklein removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Jan 19, 2022
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Comment thread content/docs/command-reference/exp/show.md Outdated
Comment thread content/docs/command-reference/exp/show.md Outdated
@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Jan 27, 2022

@karajan1001 please remember to use branches directly on this repo instead of your repo fork so the PRs are automatically deployed to review apps, and so maintainers can commit suggestions. Maybe delete your fork to avoid future confusion? Thanks

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-add-rev-to-exp--lvneuz January 27, 2022 04:23 Inactive
@jorgeorpinel jorgeorpinel changed the title exp show: add --rev flag exp show exp show: add --rev option Jan 27, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz January 27, 2022 11:16 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz January 27, 2022 11:16 Inactive
@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Jan 28, 2022

OK so I approved #3228 which is a correctly formatted version of this PR. But I'm not sure we want to merge it yet.

Is the --num <num> behavior where -1 is accepted going to be released or revisited? Thanks

In reference to discussion currently under #3191 (review)

@karajan1001
Copy link
Copy Markdown
Contributor Author

#3228

We can wait until -1 has been decided.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Looks like there's an even deeper question around -n. See treeverse/dvc#7245 (review)

If you don't mind merging branch restyled/add_rev_to_exp_show into here for now, that would be great. Thanks @karajan1001

@karajan1001

This comment was marked as resolved.

@jorgeorpinel

This comment was marked as resolved.

@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz February 4, 2022 02:45 Inactive
Comment thread content/docs/command-reference/exp/show.md Outdated
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz February 16, 2022 03:58 Inactive
Comment thread content/docs/command-reference/exp/show.md Outdated
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz February 16, 2022 07:23 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz February 16, 2022 07:29 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz February 16, 2022 07:33 Inactive
Comment on lines +80 to +82
- `-n <num>`, `--num <num>` - show experiments from the previous `num` commits
(before the `--rev` baseline). For example `-n -2` includes experiments from
`HEAD` and its 2 first ancestors.
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.

The info about negative values seems wrong here after all (sorry I was confused). The text should be closer to treeverse/dvc#7245 (review), right? Please double check the suggestion, thanks!

Suggested change
- `-n <num>`, `--num <num>` - show experiments from the previous `num` commits
(before the `--rev` baseline). For example `-n -2` includes experiments from
`HEAD` and its 2 first ancestors.
- `-n <num>`, `--num <num>` - show experiments from the `--rev` baseline and
from `num` commits before it (first parents). Give a negative value to include
all first-parent commits (similar to `git log -n`).

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Mar 8, 2022

Choose a reason for hiding this comment

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

But TBH I'm not sure why this is being changed here if there's no change for --num in the related core PR, treeverse/dvc#7204 🤷🏼

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Mar 8, 2022

Choose a reason for hiding this comment

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

Does this PR also depend on treeverse/dvc#7245 ? Marked as waiting for core merge for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But TBH I'm not sure why this is being changed here if there's no change for --num in the related core PR, iterative/dvc#7204 🤷🏼

I just forgot to update the doc in treeverse/dvc#7204.

@jorgeorpinel jorgeorpinel added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Mar 8, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz March 8, 2022 07:14 Inactive
@iesahin
Copy link
Copy Markdown
Contributor

iesahin commented Mar 8, 2022

Both of the dependency PRs seem to be merged, could we merge this now? @karajan1001

@daavoo daavoo removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Mar 8, 2022
Comment thread content/docs/command-reference/exp/show.md Outdated
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz March 9, 2022 03:04 Inactive
1. add a new flag `--rev` to `dvc exp show` command.
related to treeverse/dvc#7204

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Dave Berenbaum <dave.berenbaum@gmail.com>
@karajan1001 karajan1001 force-pushed the add_rev_to_exp_show branch from 0263218 to 06ba918 Compare March 9, 2022 03:13
@shcheklein shcheklein temporarily deployed to dvc-org-add-rev-to-exp--lvneuz March 9, 2022 03:13 Inactive
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

@jorgeorpinel jorgeorpinel removed the C: ref Content of /doc/*-reference label Mar 9, 2022
@jorgeorpinel
Copy link
Copy Markdown
Contributor

Please merge when you think it's appropriate @karajan1001 @dberenbaum (guessing the feature isn't released yet).

karajan1001 added a commit to karajan1001/dvc that referenced this pull request Mar 10, 2022
@karajan1001
Copy link
Copy Markdown
Contributor Author

You’re not authorized to merge this pull request.
Looks like I can't merge it myself @jorgeorpinel

@jorgeorpinel jorgeorpinel merged commit a6fc0fb into treeverse:master Mar 10, 2022
karajan1001 added a commit to treeverse/dvc that referenced this pull request Mar 10, 2022
fix: #7444

from the discussion treeverse/dvc.org#3187 (comment)

1. Update the help message of `-n`
@karajan1001 karajan1001 deleted the add_rev_to_exp_show branch March 10, 2022 09:12
iesahin pushed a commit that referenced this pull request Apr 11, 2022
1. add a new flag `--rev` to `dvc exp show` command.
related to treeverse/dvc#7204

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Dave Berenbaum <dave.berenbaum@gmail.com>

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Dave Berenbaum <dave.berenbaum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants