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

Clean up remotes's exps #6471

Merged
merged 18 commits into from
Sep 5, 2021
Merged

Clean up remotes's exps #6471

merged 18 commits into from
Sep 5, 2021

Conversation

karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Aug 23, 2021

fix #6006

  1. add a new argument --git-remote to dvc exp remove
  2. add some tests for it

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

fix iterative#6006
1. add a new argument `--git-remote` to `dvc exp remove`
2. add some tests for it
@karajan1001 karajan1001 requested a review from a team as a code owner August 23, 2021 10:47
@karajan1001 karajan1001 requested a review from pared August 23, 2021 10:47
@karajan1001
Copy link
Contributor Author

@dberenbaum @pmrowla
Should I support a more granular remove like in dvc exp push. The current version will clean up all of the exps in the remove repo.

@pmrowla pmrowla self-requested a review August 23, 2021 10:53
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Aug 23, 2021
@dberenbaum
Copy link
Collaborator

@dberenbaum @pmrowla
Should I support a more granular remove like in dvc exp push. The current version will clean up all of the exps in the remove repo.

Yes, I think granular removal is needed here. My preferred usage would be to make --git-remote act like the existing syntax but operating on the remote git repo instead of the local one:

  • dvc exp remove --git-remote origin abc123 removes experiment abc123 from origin.
  • dvc exp remove --git-remote origin --all removes all experiments from origin.
  • dvc exp remove --git-remote origin --queue does nothing or throws an error.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 24, 2021

I think argument/flag consistency is a guideline in https://clig.dev/

Thinking about it, I'm a bit confused about the command semantics. dvc push/pull use the default DVC remote or accept a --remote otherwise. dvc exp push/pull/remove however expect a DVC remote as argument. Seems inconsistent.

Maybe the argument these last accept should be the git remote (perhaps made optional if the branch has a default upstream), resembling git push/pull. And then use the default DVC remote or take one via -r. Cc @dberenbaum

@pmrowla
Copy link
Contributor

pmrowla commented Aug 24, 2021

Thinking about it, I'm a bit confused about the command semantics. dvc push/pull use the default DVC remote or accept a --remote otherwise. dvc exp push/pull/remove however expect a remote as argument. Seems inconsistent.

The mandatory argument for exp push/pull is the git remote. DVC data for the experiments is pushed to the default DVC remote if -r/--remote is not provided, same as dvc push/pull.

Maybe the argument these last accept should be the git remote (perhaps made optional if the branch has a default upstream), resembling git push/pull. And then use the default DVC remote or take one via -r. Cc @dberenbaum

Git branch upstream/tracking remotes are specifically only for git branch refs, they don't apply to any other git ref (including experiments). The reason we require explicit git remote arguments is because the "correct" git behavior for "default remotes" will end up being confusing for users.

1. support remote a special remote exp
2. modify the tests for it
3. fix an issue in dvc exp pull
dvc/repo/experiments/utils.py Outdated Show resolved Hide resolved
1. fix iterative#6421.
2. add a test for it.
3. do some refactors
tests/conftest.py Outdated Show resolved Hide resolved
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Sep 2, 2021
```
import file mismatch:
imported module 'test_utils' has this __file__ attribute:
  /home/runner/work/dvc/dvc/tests/unit/repo/experiments/test_utils.py
which is not the same as the test file we want to collect:
  /home/runner/work/dvc/dvc/tests/unit/stage/test_utils.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
```
@karajan1001
Copy link
Contributor Author

If

  1. there is no __init__.py in the the pytest package ( directory ).
  2. Two test file share a same file name.
    There would be a failure like this.
import file mismatch:
imported module 'test_utils' has this __file__ attribute:
  /home/runner/work/dvc/dvc/tests/unit/repo/experiments/test_utils.py
which is not the same as the test file we want to collect:
  /home/runner/work/dvc/dvc/tests/unit/stage/test_utils.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

@pmrowla
Copy link
Contributor

pmrowla commented Sep 3, 2021

Looks good, thanks for being patient with all the review requests πŸ‘

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.

One minor issue with the arg parsing. Otherwise looks great!

dvc/command/experiments.py Outdated Show resolved Hide resolved
@karajan1001 karajan1001 merged commit 4cc0633 into iterative:master Sep 5, 2021
@karajan1001 karajan1001 deleted the fix6006 branch September 5, 2021 02:06
@efiop efiop added A: experiments Related to dvc exp feature is a feature labels Sep 6, 2021
karajan1001 added a commit to karajan1001/dvc.org that referenced this pull request Oct 15, 2021
jorgeorpinel added a commit to iterative/dvc.org that referenced this pull request Oct 20, 2021
* Remove exps in remote git repo.

related to iterative/dvc#6471

* Add an example to it

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

Co-authored-by: Jorge Orpinel <[email protected]>

* Update doc according to core PR

* ref: exp remove -g updates

* Some format problems

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

Co-authored-by: Jorge Orpinel <[email protected]>
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 feature is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove pushed experiment from git remote
5 participants