Skip to content

Conversation

@karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Jan 11, 2022

wait for #7245

exp push/pull: make all the flags in exp show and exp list support pull/push (#7154)

fix: #7154

  1. add all flags (A/--all-commits , -a/--all-branches, --all-tags) in exp show to the exp pull/push
  2. add unit and func tests for exp push/pull

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 11, 2022 11:52
@karajan1001 karajan1001 changed the title exp push/pull: make all the flags in exp show and exp list support pull/push (#7154) [WIP] exp push/pull: make all the flags in exp show and exp list support pull/push (#7154) Jan 11, 2022
@karajan1001 karajan1001 marked this pull request as draft January 11, 2022 11:52
@karajan1001 karajan1001 added A: experiments Related to dvc exp enhancement Enhances DVC labels Jan 11, 2022
@karajan1001 karajan1001 force-pushed the fix7154 branch 4 times, most recently from 5bd915c to 71a4235 Compare January 17, 2022 07:26
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 17, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 18, 2022
@karajan1001 karajan1001 force-pushed the fix7154 branch 2 times, most recently from 9e58374 to b4efc61 Compare January 18, 2022 09:45
@karajan1001 karajan1001 force-pushed the fix7154 branch 3 times, most recently from e311ded to 25aa390 Compare January 24, 2022 07:53
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 24, 2022
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Jan 24, 2022
@karajan1001
Copy link
Contributor Author

Demos for the push
asciicast
for the pull
asciicast

@karajan1001 karajan1001 marked this pull request as ready for review January 25, 2022 12:06
@dberenbaum
Copy link
Contributor

Nice work @karajan1001! I have a few questions:

  1. Should there be an error if dvc exp push/pull are run without any arguments?

  2. When there are no experiments to push, the output doesn't look right:

$ dvc exp push --no-cache myremote
Pushed experiment '[]'to Git remote 'myremote'.

To push cached outputs for this experiment to DVC remote storage,re-run this command without '--no-cache'.

We should recognize that no experiments have actually been pushed and make the output something like No experiments pushed.

  1. num only seems to work when combined with --rev. Should it also be able to work from the current commit (like dvc exp show -n 2)?

@karajan1001
Copy link
Contributor Author

  1. Should there be an error if dvc exp push/pull are run without any arguments?

In my opinion, For exp push/pull/remove/gc they are not a frequent cmd and will cause exp status changes, users must explicitly give the exps to be operated on, and should raise error if none arguments are given.

  1. When there are no experiments to push, the output doesn't look right:
$ dvc exp push --no-cache myremote
Pushed experiment '[]'to Git remote 'myremote'.

To push cached outputs for this experiment to DVC remote storage,re-run this command without '--no-cache'.

We should recognize that no experiments have actually been pushed and make the output something like No experiments pushed.

Yeah.

  1. num only seems to work when combined with --rev. Should it also be able to work from the current commit (like dvc exp show -n 2)?

Yes, it works, dvc exp show -n 2 will show the exps derive from HEAD and HEAD~

@dberenbaum
Copy link
Contributor

In my opinion, For exp push/pull/remove/gc they are not a frequent cmd and will cause exp status changes, users must explicitly give the exps to be operated on, and should raise error if none arguments are given.

👍

We should recognize that no experiments have actually been pushed and make the output something like No experiments pushed.

Yeah.

👍

  1. num only seems to work when combined with --rev. Should it also be able to work from the current commit (like dvc exp show -n 2)?

Yes, it works, dvc exp show -n 2 will show the exps derive from HEAD and HEAD~

It works for me in dvc exp show but not in these other commands. For example:

$ dvc exp pull -n 2 myremote
Pulled experiment '[]' from Git remote 'myremote'.
$ dvc exp pull --rev HEAD -n 2 myremote
Pulled experiment '['exp-45e9f']' from Git remote 'myremote'.

@karajan1001
Copy link
Contributor Author

karajan1001 commented Jan 27, 2022

In my opinion, For exp push/pull/remove/gc they are not a frequent cmd and will cause exp status changes, users must explicitly give the exps to be operated on, and should raise error if none arguments are given.

👍

We should recognize that no experiments have actually been pushed and make the output something like No experiments pushed.

Yeah.

👍

  1. num only seems to work when combined with --rev. Should it also be able to work from the current commit (like dvc exp show -n 2)?

Yes, it works, dvc exp show -n 2 will show the exps derive from HEAD and HEAD~

It works for me in dvc exp show but not in these other commands. For example:

$ dvc exp pull -n 2 myremote
Pulled experiment '[]' from Git remote 'myremote'.
$ dvc exp pull --rev HEAD -n 2 myremote
Pulled experiment '['exp-45e9f']' from Git remote 'myremote'.

Excuse me, Are you in the latest version? I had already removed the default HEAD support. And this is the latest result from my computer.

> dvc exp pull -n 2 myremote
ERROR: Either of `experiment`, `--all-commits` or `--rev` needs to be set.
> dvc exp pull --rev HEAD -n 2 origin
Pulled experiment '['exp0.15', 'exp0.25', 'exp0.3', 'exp0.1']' from Git remote 'origin'.
>  dvc exp pull --rev HEAD~~ origin
No experiments to pull.

@dberenbaum
Copy link
Contributor

Sorry, must have been behind. Looks good now! One last thing:

$ dvc exp pull
ERROR: the following arguments are required: <git_remote>, <experiment>

Should <experiment> be included as a required argument?

@karajan1001
Copy link
Contributor Author

karajan1001 commented Jan 28, 2022

Sorry, must have been behind. Looks good now! One last thing:

$ dvc exp pull
ERROR: the following arguments are required: <git_remote>, <experiment>

Should <experiment> be included as a required argument?

This error comes from argparse I had set made experiment to be * which means it can be 0 to any integer, but looks like argparse can not automatically know it is not required. I tried on my computer, add a default value for the arguments can solve this problem.

@dberenbaum
Copy link
Contributor

add a default value for the arguments can solve this problem

Can the default be None?

@karajan1001
Copy link
Contributor Author

add a default value for the arguments can solve this problem

Can the default be None?

I had tried several cases on my computer. It works fine for me.

@karajan1001 karajan1001 force-pushed the fix7154 branch 2 times, most recently from a9ec406 to 9d0cced Compare February 10, 2022 08:54
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Feb 16, 2022
related to treeverse/dvc#7255

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@karajan1001 karajan1001 requested review from jorgeorpinel and skshetry and removed request for skshetry March 8, 2022 11:43
Copy link
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.

UI strings LGTM 👍🏼

…evs (treeverse#7154)

fix: treeverse#7154
1. rename flags `-A/--all-commits` in the `exp pull/push`
2. add new flag "--rev" and "--num" in the `exp pull/push`
3. Unify the collection of revs in `exp push/pull`
4. add unit and func tests for `exp push/pull`
@karajan1001
Copy link
Contributor Author

@pmrowla This one needs to be merged first before #7275.

@karajan1001 karajan1001 merged commit 244637e into treeverse:main Mar 9, 2022
@karajan1001 karajan1001 deleted the fix7154 branch March 9, 2022 09:47
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Apr 11, 2022
* exp show: add `--rev`, `-n`, `-A` flag exp pull/push

related to treeverse/dvc#7255

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

* some updates

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

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

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

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

* Restyled by prettier

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

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

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

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

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
snorkelopstesting3-bot pushed a commit to snorkel-marlin-repos/iterative_dvc_pr_7255_bbd5d522-dda3-405a-967c-72d6de89cefa that referenced this pull request Oct 22, 2025
…ll/push (#7154)

Original PR #7255 by karajan1001
Original: treeverse/dvc#7255
snorkelopstesting4-web added a commit to snorkel-marlin-repos/iterative_dvc_pr_7255_bbd5d522-dda3-405a-967c-72d6de89cefa that referenced this pull request Oct 22, 2025
…and exp list support pull/push (#7154)

Merged from original PR #7255
Original: treeverse/dvc#7255
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

make all the flags in exp show and exp list support pull/push

5 participants