Skip to content

exp list: add the new flags (including -A/--all-commits) (#7153)#7245

Merged
karajan1001 merged 1 commit into
treeverse:mainfrom
karajan1001:fix7153
Mar 8, 2022
Merged

exp list: add the new flags (including -A/--all-commits) (#7153)#7245
karajan1001 merged 1 commit into
treeverse:mainfrom
karajan1001:fix7153

Conversation

@karajan1001
Copy link
Copy Markdown
Contributor

@karajan1001 karajan1001 commented Jan 10, 2022 •
edited
Loading

fix: #7153
Unify the flags in exp list to exp show (including -A/--all-commits
)
Wait #7152 and #7206

  1. Add new -A,-n flags to exp list command.
  2. Add unit test for the flag parse.
  3. unify the revs retrive logic to exp show.
  4. Add functional test for exp list

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@karajan1001 karajan1001 requested a review from a team as a code owner January 10, 2022 08:26
@karajan1001 karajan1001 requested a review from pared January 10, 2022 08:26
@karajan1001 karajan1001 marked this pull request as draft January 10, 2022 08:26
@karajan1001 karajan1001 requested review from pmrowla and removed request for pared January 10, 2022 08:26
@karajan1001 karajan1001 force-pushed the fix7153 branch 3 times, most recently from 4996a9c to ce68fea Compare January 11, 2022 11:44
This was referenced Jan 11, 2022
Merged
Merged
@karajan1001 karajan1001 added A: experiments Related to dvc exp enhancement Enhances DVC labels Jan 11, 2022
@karajan1001 karajan1001 marked this pull request as ready for review January 11, 2022 14:24
@karajan1001 karajan1001 changed the title [WIP] exp list: add the new flags (including -A/--all-commits , -a/--all-branches, --all-tags) (#7153) exp list: add the new flags (including -A/--all-commits , -a/--all-branches, --all-tags) (#7153) Jan 11, 2022
karajan1001
karajan1001 commented Jan 12, 2022
View reviewed changes
Comment thread tests/func/experiments/test_remote.py Outdated
@karajan1001 karajan1001 force-pushed the fix7153 branch 2 times, most recently from 3f93209 to 4ccc0ce Compare January 14, 2022 10:35
@karajan1001 karajan1001 changed the title exp list: add the new flags (including -A/--all-commits , -a/--all-branches, --all-tags) (#7153) exp list: add the new flags (including -A/--all-commits) (#7153) Jan 14, 2022
@karajan1001 karajan1001 force-pushed the fix7153 branch 3 times, most recently from 9c5b39e to 338b4e6 Compare January 17, 2022 04:00
pmrowla
pmrowla previously approved these changes Jan 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

This will need a docs PR.

@karajan1001 it would also be helpful to put together an asciinema demo of the new flags (it would be fine to have one demo containing all of the different commands instead of a separate demo for each of these PRs)

@karajan1001 karajan1001 mentioned this pull request Jan 17, 2022
Merged
2 tasks
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 17, 2022
@karajan1001
add a new num arg to exp list, rename --all to --all-commits
952bd69 
Related to treeverse/dvc#7245
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 18, 2022
@karajan1001
add a new num arg to exp list, rename --all to --all-commits
f5e1e15 
Related to treeverse/dvc#7245
@karajan1001 karajan1001 mentioned this pull request Jan 18, 2022
Merged
@jorgeorpinel jorgeorpinel mentioned this pull request Jan 27, 2022
Merged
@skshetry skshetry removed their request for review January 27, 2022 03:51
This was referenced Jan 27, 2022
Merged
Merged
jorgeorpinel
jorgeorpinel reviewed Jan 27, 2022
View reviewed changes
Comment thread dvc/commands/experiments/__init__.py Outdated
jorgeorpinel
jorgeorpinel reviewed Feb 2, 2022
View reviewed changes
Comment thread dvc/commands/experiments/__init__.py Outdated
@karajan1001 karajan1001 force-pushed the fix7153 branch 3 times, most recently from 9e1a4f1 to 20ef402 Compare February 10, 2022 08:54
@jorgeorpinel jorgeorpinel mentioned this pull request Feb 16, 2022
Closed
jorgeorpinel
jorgeorpinel reviewed Feb 16, 2022
View reviewed changes
Comment thread dvc/commands/experiments/__init__.py Outdated
Comment on lines 95 to 98

This comment was marked as outdated.

Sign in to view

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, I have a question: what happens when --rev (or an included parent) is a merge? Which <num> ancestors are included?

Copy link
Copy Markdown
Contributor

@pmrowla pmrowla Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jorgeorpinel

It will include the first parent for merge commits (linear ancestors, equivalent to using tilde ~ in git). We don't support specifying a different parent (i.e carat ^ in git).

So for --rev HEAD -n 2 You would get the commit referenced by HEAD~2 in git (as opposed to HEAD~2^2/HEAD~2^3...

see: https://stackoverflow.com/a/43046393/1538451

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Got it, thanks @pmrowla ! I wonder if we want to include this info. in the description or in docs. May be relevant if people use large -n values (likely to include merges) cc @karajan1001 .

karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Feb 16, 2022
@karajan1001
add a new num arg to exp list, rename --all to --all-commits
f79c966 
Related to treeverse/dvc#7245
@dberenbaum
Copy link
Copy Markdown
Contributor

dberenbaum commented Feb 23, 2022

@karajan1001 What's the status of this one? Seems like this one needs to be merged before moving forward with the related core and docs PRs.

@karajan1001
Copy link
Copy Markdown
Contributor Author

karajan1001 commented Feb 24, 2022 •
edited
Loading

@jorgeorpinel, Need some final check and approval. the other two one #7275, #7255 all depend on it.

@dberenbaum
Copy link
Copy Markdown
Contributor

dberenbaum commented Mar 1, 2022

@karajan1001 I still see that -n -1 fails, as mentioned in #7204 (comment). I also thought that change was going to be rolled back, but I don't see any rollback. What's the status?

@karajan1001
Copy link
Copy Markdown
Contributor Author

karajan1001 commented Mar 3, 2022

@karajan1001 I still see that -n -1 fails, as mentioned in #7204 (comment). I also thought that change was going to be rolled back, but I don't see any rollback. What's the status?

need to release a new version.
treeverse/scmrepo#36

@skshetry skshetry mentioned this pull request Mar 7, 2022
Merged
jorgeorpinel
jorgeorpinel reviewed Mar 8, 2022
View reviewed changes
Comment thread dvc/commands/experiments/__init__.py Outdated
jorgeorpinel
jorgeorpinel approved these changes Mar 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Output strings look good, thanks.

@karajan1001 @jorgeorpinel
exp list: add the new flags (treeverse#7153)
0be8c85 
fix: treeverse#7153
Unify the flags in `exp list` to `exp show`

1. Add new `-A`, `-n` flags to `exp list` command.
2. Add unit test for the flag parse.
3. unify the revs retrive logic to `exp show`.
4. Add functional test for `exp list`
5. Update the documents.

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@karajan1001 karajan1001 enabled auto-merge (rebase) March 8, 2022 07:52
@karajan1001 karajan1001 merged commit d033345 into treeverse:main Mar 8, 2022
@karajan1001 karajan1001 deleted the fix7153 branch March 8, 2022 08:18
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Mar 30, 2022
@karajan1001 @jorgeorpinel @restyled-commits
exp list: add --num option, rename --all to --all-commits (#3191
9452b6e 
)

* add a new `num` arg to exp list, rename `--all` to `--all-commits`

Related to treeverse/dvc#7245

* Some updates

* Update content/docs/command-reference/exp/list.md

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

* Restyled by prettier

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
iesahin pushed a commit to treeverse/dvc.org that referenced this pull request Apr 11, 2022
@karajan1001 @jorgeorpinel
@restyled-commits @iesahin
exp list: add --num option, rename --all to --all-commits (#3191
05e3945 
)

* add a new `num` arg to exp list, rename `--all` to `--all-commits`

Related to treeverse/dvc#7245

* Some updates

* Update content/docs/command-reference/exp/list.md

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

* Restyled by prettier

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@iesahin iesahin iesahin left review comments

@dberenbaum dberenbaum dberenbaum left review comments

@pmrowla pmrowla pmrowla approved these changes

@jorgeorpinel jorgeorpinel jorgeorpinel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A: experiments Related to dvc exp enhancement Enhances DVC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add the new flags (including -A/--all-commits, -a/--all-branches, --all-tags) to exp list

5 participants

@karajan1001 @pmrowla @dberenbaum @iesahin @jorgeorpinel