dvc exp remove --queue -A: possibly incomplete result returned

[rmic]

Bug Report

Description

As discussed in #10633 (comment),
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):

dvc/dvc/repo/experiments/remove.py

Lines 69 to 71 in d38b2db

	elif all_commits:
	exp_ref_list.extend(exp_refs(repo.scm, git_remote))
	removed = [ref.name for ref in exp_ref_list]

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.

[shcheklein]

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?

[rmic]

The place where this seems the most visible is in

dvc/dvc/commands/experiments/remove.py

Lines 30 to 39 in d38b2db

	removed = self.repo.experiments.remove(
	exp_names=self.args.experiment,
	all_commits=self.args.all_commits,
	rev=self.args.rev,
	num=self.args.num,
	queue=self.args.queue,
	git_remote=self.args.git_remote,
	)
	if removed:
	ui.write(f"Removed experiments: {humanize.join(map(repr, removed))}")

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 ), 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.

[shcheklein]

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 !