Repository: iterative/dvc
Pull request: Clean up remotes's exps
fix #6006 

1. add a new argument `--git-remote` to `dvc exp remove`
2. add some tests for it

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [x] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.
  https://github.com/iterative/dvc.org/pull/2744

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
Remove pushed experiment from git remote
Push an experiment to a git remote:

```
$ dvc exp push origin myexpe
```

Then:

```
$ dvc exp list origin
master:
   myexpe
```

Currently, only local experiment can be removed with:

```
$ dvc exp remove myexpe
```

but `dvc exp list origin` still shows `myexpe`.

I would like to also be able to remove a pushed experiment on the remote.

Hints from issue discussion:
For now you can just use git and directly remove experiment refs with `git push -d` in the same way that you remove remote branches

```shell
# get list of exps pushed to origin
$ git ls-remote origin "refs/exps/*"

# remove one of the listed refs
$ git push -d origin refs/exps/path/to/ref
```
@pmrowla Do you think there's anything to discuss here, or is this a straightforward feature to implement?
@dberenbaum it still needs a discussion of what the usage should look like
I think we actually need options for both `dvc exp remove --git-remote` (as mentioned above) and `dvc exp gc --git-remote` (to clean up lots of old experiments that aren't relevant anymore).

> @dberenbaum it still needs a discussion of what the usage should look like

Can you elaborate? Does the suggested syntax above help?
@dberenbaum My original comment was regarding whether it should be tied to `exp remove/gc` or whether it should look more like git's `push -d` workflow.

Also, I do think it's worth asking the question of "is removing remote experiments necessary"? There are reasons to disallow it - the same reasons that github always keeps all pull request refs and always keeps references to all remote git branches that have ever existed (including remote branches that users manually "delete" through either github's UI or the git CLI).

Should removing remote exps actually delete refs, or should it just hide them in the way that github "removes" git branches - they are just moved into a different namespace so they don't show up in git CLI "list remote branch" commands (so that they can be restored later if the user decides they didn't actually want to delete that branch)?

Or maybe what we really need is just a way to filter the results of `exp list <git remote>` in the first place, instead adding any remote remove functionality at all?
Also I know this came up in the CML auto-push discussion as well (since we will be generating a lot of experiments that way). Maybe the CML runner pushed exps should just go into their own namespace that can be regularly cleaned on the CML side
> My original comment was regarding whether it should be tied to `exp remove/gc` or whether it should look more like git's `push -d` workflow.
> 
> Also, I do think it's worth asking the question of "is removing remote experiments necessary"? There are reasons to disallow it - the same reasons that github always keeps all pull request refs and always keeps references to all remote git branches that have ever existed (including remote branches that users manually "delete" through either github's UI or the git CLI).

IMO we don't need to follow Git since `dvc exp` provides a lightweight tracking without the strict (but useful) constraints of version control. If I want full version control, I'll use `dvc exp branch/apply` and regular Git branches. Experiments are intentionally ephemeral, so removing them seems a natural and necessary feature, or else we are just duplicating existing Git behavior.

Similarly, I think extending the existing dvc commands for removing experiments seems more natural than trying to follow the `git push -d` syntax.

> Should removing remote exps actually delete refs, or should it just hide them in the way that github "removes" git branches - they are just moved into a different namespace so they don't show up in git CLI "list remote branch" commands (so that they can be restored later if the user decides they didn't actually want to delete that branch)?

Given the ephemeral nature of experiments, I think it's fine to delete them and maybe preferable to moving them. People might accidentally push a huge experiment and want to delete the ref so they can gc the repo and clean up hanging commits. That's probably a separate issue, but deleting the refs seems acceptable to me.

> Also I know this came up in the CML auto-push discussion as well (since we will be generating a lot of experiments that way). Maybe the CML runner pushed exps should just go into their own namespace that can be regularly cleaned on the CML side

Good idea. Either approach could be useful for CML, but that's not the primary driver for this feature. 

We are starting to promote experiment sharing more in docs, courses, etc., and it seems like sharing experiments isn't ready to be used if we don't have some way to remove pushed experiments. Experiments should be easy to both create and delete so that users feel free to try lots of things and only keep what works, regardless of whether the experiments are local or shared.
regarding implementation for this, deleting remote refs is a `git push -d` operation, internally this needs to be done via 
https://github.com/iterative/dvc/blob/82e559a51a3bb92e83fd262df68d0d762fbf2b28/dvc/scm/git/backend/base.py#L193
with `src` set to None

Commit message: Clean up remotes's exps

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

Patch to review:
diff --git a/dvc/command/experiments.py b/dvc/command/experiments.py
index e1fa5d83f1..eafecda500 100644
--- a/dvc/command/experiments.py
+++ b/dvc/command/experiments.py
@@ -750,6 +750,7 @@ def run(self):
             exp_names=self.args.experiment,
             queue=self.args.queue,
             clear_all=self.args.all,
+            remote=self.args.git_remote,
         )
 
         return 0
@@ -1249,6 +1250,12 @@ def add_parser(subparsers, parent_parser):
         action="store_true",
         help="Remove all committed experiments.",
     )
+    remove_group.add_argument(
+        "-r",
+        "--git-remote",
+        metavar="<git_remote>",
+        help="Name of the Git remote to GC all of the experiment branches.",
+    )
     experiments_remove_parser.add_argument(
         "experiment",
         nargs="*",
diff --git a/dvc/repo/experiments/remove.py b/dvc/repo/experiments/remove.py
index 2df54728bd..d14f338fe0 100644
--- a/dvc/repo/experiments/remove.py
+++ b/dvc/repo/experiments/remove.py
@@ -7,7 +7,7 @@
 from dvc.scm.base import RevError
 
 from .base import EXPS_NAMESPACE, ExpRefInfo
-from .utils import exp_refs, exp_refs_by_name, remove_exp_refs
+from .utils import exp_refs, exp_refs_by_name, remote_exp_refs, remove_exp_refs
 
 logger = logging.getLogger(__name__)
 
@@ -19,9 +19,10 @@ def remove(
     exp_names=None,
     queue=False,
     clear_all=False,
+    remote=None,
     **kwargs,
 ):
-    if not any([exp_names, queue, clear_all]):
+    if not any([exp_names, queue, clear_all, remote]):
         return 0
 
     removed = 0
@@ -29,6 +30,8 @@ def remove(
         removed += _clear_stash(repo)
     if clear_all:
         removed += _clear_all(repo)
+    if remote:
+        removed += _clear_remote(repo, remote)
 
     if exp_names:
         remained = _remove_commited_exps(repo, exp_names)
@@ -41,6 +44,14 @@ def remove(
     return removed
 
 
+def _clear_remote(repo, remote: str):
+    ref_infos = list(remote_exp_refs(repo.scm, remote))
+    for ref_info in ref_infos:
+        ref_name = str(ref_info)
+        repo.scm.push_refspec(remote, None, ref_name)
+    return len(ref_infos)
+
+
 def _clear_stash(repo):
     removed = len(repo.experiments.stash)
     repo.experiments.stash.clear()
diff --git a/dvc/scm/git/backend/dulwich.py b/dvc/scm/git/backend/dulwich.py
index 12e1691426..48a5f87988 100644
--- a/dvc/scm/git/backend/dulwich.py
+++ b/dvc/scm/git/backend/dulwich.py
@@ -399,9 +399,11 @@ def push_refspec(
             ) from exc
 
         def update_refs(refs):
+            from dulwich.objects import ZERO_SHA
+
             new_refs = {}
             for ref, value in zip(dest_refs, values):
-                if ref in refs:
+                if ref in refs and value != ZERO_SHA:
                     local_sha = self.repo.refs[ref]
                     remote_sha = refs[ref]
                     try:
diff --git a/tests/func/experiments/test_remove.py b/tests/func/experiments/test_remove.py
index 909561a080..fb7c7560d9 100644
--- a/tests/func/experiments/test_remove.py
+++ b/tests/func/experiments/test_remove.py
@@ -90,3 +90,27 @@ def test_remove_all(tmp_dir, scm, dvc, exp_stage):
     assert len(dvc.experiments.stash) == 2
     assert scm.get_ref(str(ref_info2)) is None
     assert scm.get_ref(str(ref_info1)) is None
+
+
+@pytest.mark.parametrize("use_url", [True, False])
+def test_remove_remote(tmp_dir, scm, dvc, exp_stage, git_upstream, use_url):
+    remote = git_upstream.url if use_url else git_upstream.remote
+
+    results = dvc.experiments.run(exp_stage.addressing, params=["foo=1"])
+    exp1 = first(results)
+    ref_info1 = first(exp_refs_by_rev(scm, exp1))
+
+    results = dvc.experiments.run(exp_stage.addressing, params=["foo=2"])
+    exp2 = first(results)
+    ref_info2 = first(exp_refs_by_rev(scm, exp2))
+
+    dvc.experiments.push(remote, ref_info1.name)
+    dvc.experiments.push(remote, ref_info2.name)
+    assert git_upstream.scm.get_ref(str(ref_info1)) == exp1
+    assert git_upstream.scm.get_ref(str(ref_info2)) == exp2
+
+    dvc.experiments.remove(experiments=[ref_info1])
+    dvc.experiments.remove(remote=remote)
+
+    assert git_upstream.scm.get_ref(str(ref_info1)) is None
+    assert git_upstream.scm.get_ref(str(ref_info2)) is None
diff --git a/tests/unit/command/test_experiments.py b/tests/unit/command/test_experiments.py
index d8043af94b..6c066ca304 100644
--- a/tests/unit/command/test_experiments.py
+++ b/tests/unit/command/test_experiments.py
@@ -253,15 +253,17 @@ def test_experiments_pull(dvc, scm, mocker):
 
 
 @pytest.mark.parametrize(
-    "queue,clear_all",
-    [(True, False), (False, True)],
+    "queue,clear_all,remote",
+    [(True, False, None), (False, True, None), (False, False, True)],
 )
-def test_experiments_remove(dvc, scm, mocker, queue, clear_all):
+def test_experiments_remove(dvc, scm, mocker, queue, clear_all, remote):
     if queue:
-        args = "--queue"
+        args = ["--queue"]
     if clear_all:
-        args = "--all"
-    cli_args = parse_args(["experiments", "remove", args])
+        args = ["--all"]
+    if remote:
+        args = ["--git-remote", "myremote"]
+    cli_args = parse_args(["experiments", "remove"] + args)
     assert cli_args.func == CmdExperimentsRemove
 
     cmd = cli_args.func(cli_args)
@@ -273,4 +275,5 @@ def test_experiments_remove(dvc, scm, mocker, queue, clear_all):
         exp_names=[],
         queue=queue,
         clear_all=clear_all,
+        remote="myremote" if remote else None,
     )


Code review comments:
diff_hunk: @@ -1249,6 +1250,12 @@ def add_parser(subparsers, parent_parser):
         action="store_true",
         help="Remove all committed experiments.",
     )
+    remove_group.add_argument(
+        "-r",
+        "--git-remote",
+        metavar="<git_remote>",
+        help="Name of the Git remote to GC all of the experiment branches.",
path: dvc/command/experiments.py
text: @user1:
Why GC? Isn't this `exp remove`?

@user1:
Also what's the default? `origin`, I assume. Thanks

@user2:
There is no default, if `--git-remote` is not specified it removes a local experiment ref and not a remote one

@author:
I think it is better to use "Name or URL of the Git remote to delete all of the experiment references."" 

> Also what's the default? origin, I assume. Thanks
No default for this

@user1:
> There is no default

What does `exp remove -g 1234567` do? Errors out? If so OK 👍 thanks