Repository: iterative/dvc
Issue category: Bug Report

Issue:
`dvc exp remove --queue -A`: possibly incomplete result returned
# Bug Report

<!--
## Issue name

Issue names must follow the pattern `command: description` where the command is the dvc command that you are trying to run. The description should describe the consequence of the bug.

Example: `repro: doesn't detect input changes`
-->

## Description

As discussed in https://github.com/iterative/dvc/pull/10633#discussion_r1857943029, 
when using `dvc exp remove` with both `--queue` and `-A `, an incomplete list of removed experiments might be returned: the queued ones would be missing from the returned list, even if they are effectively removed as expected.

This is just a minor inconvenience at the moment, but for the sake of correctness and code cleanliness, I'll put it here.

### Reproduce

1. create repo
2. run experiments 
3. queue experiments (`dvc exp run --queue`)
4. `dvc exp remove --queue -A`


### Expected
The list of all experiments, both queued and committed
**Additional Information (if any):**

https://github.com/iterative/dvc/blob/d38b2dbcb873b5112976c5ad40c5574b5d2a41f3/dvc/repo/experiments/remove.py#L69-L71

line 71 should use `removed.extend` just as it is done in the other if / elif code blocks

I can start working on it soon.


Hints:
thanks @rmic ! where do we use the returned value atm? E.g. does it affect the way we print the results for the `dvc exp remove` command to the screen?
The place where this seems the most visible is in 

https://github.com/iterative/dvc/blob/d38b2dbcb873b5112976c5ad40c5574b5d2a41f3/dvc/commands/experiments/remove.py#L30-L39

which is where the list of removed experiments is displayed to the user on the CLI

The returned value is probably also used in tests here and there, but as they all pass, this probably means that this combination is currently not being tested (or only with an empty queue). At least in the relevant tests I'm familiar with for remove (i.e.: [`tests/func/experiments/test_remove.py`](https://github.com/iterative/dvc/blob/d38b2dbcb873b5112976c5ad40c5574b5d2a41f3/tests/func/experiments/test_remove.py) ), there are tests that : 
- remove specific experiments from the queue
- remove all experiments from the queue
- remove specific commits 
- remove all commits 

but none that tests both all queued and all commits.

That's all I found in this codebase, I have no idea what other people might do with this value if they have built code that depend on this api to remove their experiments in their own tool.



Good, thanks. I think it makes sense to fix it to return the proper list of experiments (even to be printed properly). It's weird that if we remove queued experiments we pretty much don't print anything (?). Thanks for the detailed analysis and explanation @rmic !

Patch:
diff --git a/dvc/repo/experiments/remove.py b/dvc/repo/experiments/remove.py
index cd8ca07e8b..1b29f30255 100644
--- a/dvc/repo/experiments/remove.py
+++ b/dvc/repo/experiments/remove.py
@@ -75,7 +75,7 @@ def remove(  # noqa: C901, PLR0912
         exp_ref_list.extend(exp_ref_dict.values())
     elif all_commits:
         exp_ref_list.extend(exp_refs(repo.scm, git_remote))
-        removed = [ref.name for ref in exp_ref_list]
+        removed.extend([ref.name for ref in exp_ref_list])
 
     if keep:
         exp_ref_list = list(set(exp_refs(repo.scm, git_remote)) - set(exp_ref_list))


Test patch:
diff --git a/tests/func/experiments/test_remove.py b/tests/func/experiments/test_remove.py
index 1864cc541d..8dd2b65498 100644
--- a/tests/func/experiments/test_remove.py
+++ b/tests/func/experiments/test_remove.py
@@ -43,6 +43,26 @@ def test_remove_all_queued_experiments(tmp_dir, scm, dvc, exp_stage):
     assert scm.get_ref(str(ref_info)) is not None
 
 
+def test_remove_all_experiments_queued_and_completed(tmp_dir, scm, dvc, exp_stage):
+    queue_length = 3
+    for i in range(queue_length):
+        dvc.experiments.run(
+            exp_stage.addressing, params=[f"foo={i}"], name=f"exp{i}", queue=True
+        )
+
+    results = dvc.experiments.run(
+        exp_stage.addressing, params=[f"foo={queue_length}"], name=f"exp{queue_length}"
+    )
+    ref_info = first(exp_refs_by_rev(scm, first(results)))
+
+    removed = sorted(dvc.experiments.remove(all_commits=True, queue=True))
+
+    assert len(removed) == queue_length + 1
+    assert removed == [f"exp{i}" for i in range(queue_length)] + [ref_info.name]
+    assert len(dvc.experiments.stash_revs) == 0
+    assert scm.get_ref(str(ref_info)) is None
+
+
 def test_remove_special_queued_experiments(tmp_dir, scm, dvc, exp_stage):
     dvc.experiments.run(
         exp_stage.addressing, params=["foo=1"], queue=True, name="queue1"