From f1804354d3dd76041deddccb7ffc725f9995316d Mon Sep 17 00:00:00 2001 From: dberenbaum Date: Mon, 22 May 2023 16:40:55 -0400 Subject: [PATCH 1/4] exp list: include sha; still needs remote support --- dvc/commands/experiments/ls.py | 24 +++++++++++++++------ dvc/repo/experiments/ls.py | 12 ++++++++--- tests/func/experiments/test_experiments.py | 16 +++++++------- tests/func/experiments/test_remote.py | 6 +++--- tests/func/experiments/test_save.py | 4 ++-- tests/unit/command/test_experiments.py | 25 ++++++++++++++++++++++ 6 files changed, 65 insertions(+), 22 deletions(-) diff --git a/dvc/commands/experiments/ls.py b/dvc/commands/experiments/ls.py index 2e50b091c9..40e6f6479a 100644 --- a/dvc/commands/experiments/ls.py +++ b/dvc/commands/experiments/ls.py @@ -11,6 +11,7 @@ class CmdExperimentsList(CmdBase): def run(self): name_only = self.args.name_only + sha_only = self.args.sha_only exps = self.repo.experiments.ls( all_commits=self.args.all_commits, rev=self.args.rev, @@ -19,11 +20,15 @@ def run(self): ) for baseline in exps: - if not name_only: + if not (name_only or sha_only): ui.write(f"{baseline}:") - for exp_name in exps[baseline]: - indent = "" if name_only else "\t" - ui.write(f"{indent}{exp_name}") + for exp_name, rev in exps[baseline]: + if name_only: + ui.write(exp_name) + elif sha_only: + ui.write(rev) + else: + ui.write(f"\t{exp_name}\t{rev}") return 0 @@ -40,11 +45,18 @@ def add_parser(experiments_subparsers, parent_parser): formatter_class=argparse.RawDescriptionHelpFormatter, ) add_rev_selection_flags(experiments_list_parser, "List") - experiments_list_parser.add_argument( + display_group = experiments_list_parser.add_mutually_exclusive_group() + display_group.add_argument( "--name-only", "--names-only", action="store_true", - help="Only output experiment names (without parent commits).", + help="Only output experiment names (without SHAs or parent commits).", + ) + display_group.add_argument( + "--sha-only", + "--shas-only", + action="store_true", + help="Only output experiment commit SHAs (without names or parent commits).", ) experiments_list_parser.add_argument( "git_remote", diff --git a/dvc/repo/experiments/ls.py b/dvc/repo/experiments/ls.py index c488d19211..d52365fea6 100644 --- a/dvc/repo/experiments/ls.py +++ b/dvc/repo/experiments/ls.py @@ -1,6 +1,6 @@ import logging from collections import defaultdict -from typing import List, Optional, Union +from typing import Dict, List, Optional, Tuple, Union from dvc.repo import locked from dvc.repo.scm_context import scm_context @@ -19,7 +19,11 @@ def ls( all_commits: bool = False, num: int = 1, git_remote: Optional[str] = None, -): +) -> Dict[str, List[Tuple[str, str]]]: + """List experiments. + + Returns a dict mapping baseline revs to a list of (exp_name, exp_sha) tuples. + """ rev_set = None if not all_commits: rev = rev or "HEAD" @@ -39,6 +43,8 @@ def ls( name = baseline[:7] if tags[baseline] or ref_heads[baseline]: name = tags[baseline] or ref_heads[baseline][len(base) + 1 :] - results[name] = [info.name for info in ref_info_dict[baseline]] + for info in ref_info_dict[baseline]: + exp_rev = next(repo.scm.branch_revs(str(info), info.baseline_sha))[:7] + results[name].append((info.name, exp_rev)) return results diff --git a/tests/func/experiments/test_experiments.py b/tests/func/experiments/test_experiments.py index 7fd34c788b..e7f06423f7 100644 --- a/tests/func/experiments/test_experiments.py +++ b/tests/func/experiments/test_experiments.py @@ -362,30 +362,30 @@ def test_list(tmp_dir, scm, dvc, exp_stage): exp_c = first(results) ref_info_c = first(exp_refs_by_rev(scm, exp_c)) - assert dvc.experiments.ls() == {"master": [ref_info_c.name]} + assert dvc.experiments.ls() == {"master": [(ref_info_c.name, exp_c[:7])]} exp_list = dvc.experiments.ls(rev=ref_info_a.baseline_sha) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name} + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])} } exp_list = dvc.experiments.ls(rev=[baseline_a, scm.get_rev()]) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name}, - "master": {ref_info_c.name}, + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, + "master": {(ref_info_c.name, exp_c[:7])}, } exp_list = dvc.experiments.ls(all_commits=True) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name}, - "master": {ref_info_c.name}, + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, + "master": {(ref_info_c.name, exp_c[:7])}, } scm.checkout("branch", True) exp_list = dvc.experiments.ls(all_commits=True) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name}, - "branch": {ref_info_c.name}, + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, + "branch": {(ref_info_c.name, exp_c[:7])}, } diff --git a/tests/func/experiments/test_remote.py b/tests/func/experiments/test_remote.py index 2f09721f35..345146969a 100644 --- a/tests/func/experiments/test_remote.py +++ b/tests/func/experiments/test_remote.py @@ -179,13 +179,13 @@ def test_list_remote(tmp_dir, scm, dvc, git_downstream, exp_stage, use_url): git_downstream.tmp_dir.scm.fetch_refspecs(remote, ["master:master"]) exp_list = downstream_exp.ls(rev=baseline_a, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name} + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])} } exp_list = downstream_exp.ls(all_commits=True, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {ref_info_a.name, ref_info_b.name}, - "master": {ref_info_c.name}, + baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, + "master": {(ref_info_c.name, exp_c[:7])}, } diff --git a/tests/func/experiments/test_save.py b/tests/func/experiments/test_save.py index fb127c2436..56394b2430 100644 --- a/tests/func/experiments/test_save.py +++ b/tests/func/experiments/test_save.py @@ -70,8 +70,8 @@ def test_exp_save_after_commit(tmp_dir, dvc, scm): dvc.experiments.save(name="exp-2", force=True) all_exps = dvc.experiments.ls(all_commits=True) - assert all_exps[baseline[:7]] == ["exp-1"] - assert all_exps["master"] == ["exp-2"] + assert all_exps[baseline[:7]][0][0] == "exp-1" + assert all_exps["master"][0][0] == "exp-2" def test_exp_save_with_staged_changes(tmp_dir, dvc, scm): diff --git a/tests/unit/command/test_experiments.py b/tests/unit/command/test_experiments.py index 4b78985747..1fa4c29798 100644 --- a/tests/unit/command/test_experiments.py +++ b/tests/unit/command/test_experiments.py @@ -188,6 +188,31 @@ def test_experiments_list(dvc, scm, mocker): ) +@pytest.mark.parametrize("args", [["--name-only"], ["--sha-only"], []]) +def test_experiments_list_format(mocker, capsys, args): + mocker.patch( + "dvc.repo.experiments.ls.ls", + return_value={ + "main": [ + ("exp-a", "sha-a"), + ] + }, + ) + cli_args = parse_args( + [ + "experiments", + "list", + ] + ) + + cmd = cli_args.func(cli_args) + + capsys.readouterr() + assert cmd.run() == 0 + cap = capsys.readouterr() + assert cap.out == "main:\n\texp-a\tsha-a" + + def test_experiments_push(dvc, scm, mocker): cli_args = parse_args( [ From 4a3a4f064f8c37c5974d49dd3151d7ddde24a47b Mon Sep 17 00:00:00 2001 From: dberenbaum Date: Tue, 23 May 2023 19:55:00 -0400 Subject: [PATCH 2/4] exp list: ignore rev for remotes --- dvc/commands/experiments/ls.py | 5 ++++- dvc/repo/experiments/ls.py | 7 +++++-- tests/func/experiments/test_remote.py | 6 +++--- tests/unit/command/test_experiments.py | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/dvc/commands/experiments/ls.py b/dvc/commands/experiments/ls.py index 40e6f6479a..2e41c786c3 100644 --- a/dvc/commands/experiments/ls.py +++ b/dvc/commands/experiments/ls.py @@ -28,7 +28,10 @@ def run(self): elif sha_only: ui.write(rev) else: - ui.write(f"\t{exp_name}\t{rev}") + exp_out = f"\t{exp_name}" + if rev: + exp_out += f"\t{rev}" + ui.write(exp_out) return 0 diff --git a/dvc/repo/experiments/ls.py b/dvc/repo/experiments/ls.py index d52365fea6..7d44e4f545 100644 --- a/dvc/repo/experiments/ls.py +++ b/dvc/repo/experiments/ls.py @@ -19,7 +19,7 @@ def ls( all_commits: bool = False, num: int = 1, git_remote: Optional[str] = None, -) -> Dict[str, List[Tuple[str, str]]]: +) -> Dict[str, List[Tuple[str, Optional[str]]]]: """List experiments. Returns a dict mapping baseline revs to a list of (exp_name, exp_sha) tuples. @@ -44,7 +44,10 @@ def ls( if tags[baseline] or ref_heads[baseline]: name = tags[baseline] or ref_heads[baseline][len(base) + 1 :] for info in ref_info_dict[baseline]: - exp_rev = next(repo.scm.branch_revs(str(info), info.baseline_sha))[:7] + if git_remote: + exp_rev = None + else: + exp_rev = next(repo.scm.branch_revs(str(info), info.baseline_sha))[:7] results[name].append((info.name, exp_rev)) return results diff --git a/tests/func/experiments/test_remote.py b/tests/func/experiments/test_remote.py index 345146969a..9b8a95999f 100644 --- a/tests/func/experiments/test_remote.py +++ b/tests/func/experiments/test_remote.py @@ -179,13 +179,13 @@ def test_list_remote(tmp_dir, scm, dvc, git_downstream, exp_stage, use_url): git_downstream.tmp_dir.scm.fetch_refspecs(remote, ["master:master"]) exp_list = downstream_exp.ls(rev=baseline_a, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])} + baseline_a[:7]: {(ref_info_a.name, None), (ref_info_b.name, None)} } exp_list = downstream_exp.ls(all_commits=True, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, - "master": {(ref_info_c.name, exp_c[:7])}, + baseline_a[:7]: {(ref_info_a.name, None), (ref_info_b.name, None)}, + "master": {(ref_info_c.name, None)}, } diff --git a/tests/unit/command/test_experiments.py b/tests/unit/command/test_experiments.py index 1fa4c29798..8241dd183a 100644 --- a/tests/unit/command/test_experiments.py +++ b/tests/unit/command/test_experiments.py @@ -210,7 +210,7 @@ def test_experiments_list_format(mocker, capsys, args): capsys.readouterr() assert cmd.run() == 0 cap = capsys.readouterr() - assert cap.out == "main:\n\texp-a\tsha-a" + assert cap.out == "main:\n\texp-a\tsha-a\n" def test_experiments_push(dvc, scm, mocker): From 5080170c14c64667fcb2eb8cf85eb2c20f75aa51 Mon Sep 17 00:00:00 2001 From: dberenbaum Date: Tue, 23 May 2023 20:23:58 -0400 Subject: [PATCH 3/4] exp list: update output format --- dvc/commands/experiments/ls.py | 13 ++++--- tests/unit/command/test_experiments.py | 47 +++++++++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/dvc/commands/experiments/ls.py b/dvc/commands/experiments/ls.py index 2e41c786c3..7f48dadef0 100644 --- a/dvc/commands/experiments/ls.py +++ b/dvc/commands/experiments/ls.py @@ -3,6 +3,7 @@ from dvc.cli.command import CmdBase from dvc.cli.utils import append_doc_link +from dvc.exceptions import InvalidArgumentError from dvc.ui import ui logger = logging.getLogger(__name__) @@ -12,11 +13,14 @@ class CmdExperimentsList(CmdBase): def run(self): name_only = self.args.name_only sha_only = self.args.sha_only + git_remote = self.args.git_remote + if sha_only and git_remote: + raise InvalidArgumentError("--sha-only not supported with git_remote.") exps = self.repo.experiments.ls( all_commits=self.args.all_commits, rev=self.args.rev, num=self.args.num, - git_remote=self.args.git_remote, + git_remote=git_remote, ) for baseline in exps: @@ -27,11 +31,10 @@ def run(self): ui.write(exp_name) elif sha_only: ui.write(rev) + elif rev: + ui.write(f"\t{rev} [{exp_name}]") else: - exp_out = f"\t{exp_name}" - if rev: - exp_out += f"\t{rev}" - ui.write(exp_out) + ui.write(f"\t{exp_name}") return 0 diff --git a/tests/unit/command/test_experiments.py b/tests/unit/command/test_experiments.py index 8241dd183a..3bf3d53e42 100644 --- a/tests/unit/command/test_experiments.py +++ b/tests/unit/command/test_experiments.py @@ -188,8 +188,15 @@ def test_experiments_list(dvc, scm, mocker): ) -@pytest.mark.parametrize("args", [["--name-only"], ["--sha-only"], []]) -def test_experiments_list_format(mocker, capsys, args): +@pytest.mark.parametrize( + "args,expected", + [ + ([], "main:\n\tsha-a [exp-a]\n"), + (["--name-only"], "exp-a\n"), + (["--sha-only"], "sha-a\n"), + ], +) +def test_experiments_list_format(mocker, capsys, args, expected): mocker.patch( "dvc.repo.experiments.ls.ls", return_value={ @@ -198,19 +205,43 @@ def test_experiments_list_format(mocker, capsys, args): ] }, ) - cli_args = parse_args( - [ - "experiments", - "list", - ] + raw_args = ["experiments", "list", *args] + cli_args = parse_args(raw_args) + + cmd = cli_args.func(cli_args) + + capsys.readouterr() + assert cmd.run() == 0 + cap = capsys.readouterr() + assert cap.out == expected + + +def test_experiments_list_remote(mocker, capsys): + mocker.patch( + "dvc.repo.experiments.ls.ls", + return_value={ + "main": [ + ("exp-a", None), + ] + }, ) + cli_args = parse_args(["experiments", "list", "git_remote"]) cmd = cli_args.func(cli_args) capsys.readouterr() assert cmd.run() == 0 cap = capsys.readouterr() - assert cap.out == "main:\n\texp-a\tsha-a\n" + assert cap.out == "main:\n\texp-a\n" + + cli_args = parse_args(["experiments", "list", "git_remote", "--sha-only"]) + + cmd = cli_args.func(cli_args) + + capsys.readouterr() + + with pytest.raises(InvalidArgumentError): + cmd.run() def test_experiments_push(dvc, scm, mocker): From c76ca23680f718db9f34b1da1b3f7a026acd803c Mon Sep 17 00:00:00 2001 From: dberenbaum Date: Wed, 24 May 2023 11:43:31 -0400 Subject: [PATCH 4/4] refactor --- dvc/commands/experiments/ls.py | 4 ++-- dvc/repo/experiments/ls.py | 4 ++-- tests/func/experiments/test_experiments.py | 16 ++++++++-------- tests/func/experiments/test_remote.py | 4 ++-- tests/func/experiments/test_save.py | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/dvc/commands/experiments/ls.py b/dvc/commands/experiments/ls.py index 7f48dadef0..fcc1456a6d 100644 --- a/dvc/commands/experiments/ls.py +++ b/dvc/commands/experiments/ls.py @@ -25,14 +25,14 @@ def run(self): for baseline in exps: if not (name_only or sha_only): - ui.write(f"{baseline}:") + ui.write(f"{baseline[:7]}:") for exp_name, rev in exps[baseline]: if name_only: ui.write(exp_name) elif sha_only: ui.write(rev) elif rev: - ui.write(f"\t{rev} [{exp_name}]") + ui.write(f"\t{rev[:7]} [{exp_name}]") else: ui.write(f"\t{exp_name}") diff --git a/dvc/repo/experiments/ls.py b/dvc/repo/experiments/ls.py index 7d44e4f545..3a8cebd4a1 100644 --- a/dvc/repo/experiments/ls.py +++ b/dvc/repo/experiments/ls.py @@ -40,14 +40,14 @@ def ls( results = defaultdict(list) for baseline in ref_info_dict: - name = baseline[:7] + name = baseline if tags[baseline] or ref_heads[baseline]: name = tags[baseline] or ref_heads[baseline][len(base) + 1 :] for info in ref_info_dict[baseline]: if git_remote: exp_rev = None else: - exp_rev = next(repo.scm.branch_revs(str(info), info.baseline_sha))[:7] + exp_rev = repo.scm.get_ref(str(info)) results[name].append((info.name, exp_rev)) return results diff --git a/tests/func/experiments/test_experiments.py b/tests/func/experiments/test_experiments.py index e7f06423f7..fbfba81768 100644 --- a/tests/func/experiments/test_experiments.py +++ b/tests/func/experiments/test_experiments.py @@ -362,30 +362,30 @@ def test_list(tmp_dir, scm, dvc, exp_stage): exp_c = first(results) ref_info_c = first(exp_refs_by_rev(scm, exp_c)) - assert dvc.experiments.ls() == {"master": [(ref_info_c.name, exp_c[:7])]} + assert dvc.experiments.ls() == {"master": [(ref_info_c.name, exp_c)]} exp_list = dvc.experiments.ls(rev=ref_info_a.baseline_sha) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])} + baseline_a: {(ref_info_a.name, exp_a), (ref_info_b.name, exp_b)} } exp_list = dvc.experiments.ls(rev=[baseline_a, scm.get_rev()]) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, - "master": {(ref_info_c.name, exp_c[:7])}, + baseline_a: {(ref_info_a.name, exp_a), (ref_info_b.name, exp_b)}, + "master": {(ref_info_c.name, exp_c)}, } exp_list = dvc.experiments.ls(all_commits=True) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, - "master": {(ref_info_c.name, exp_c[:7])}, + baseline_a: {(ref_info_a.name, exp_a), (ref_info_b.name, exp_b)}, + "master": {(ref_info_c.name, exp_c)}, } scm.checkout("branch", True) exp_list = dvc.experiments.ls(all_commits=True) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, exp_a[:7]), (ref_info_b.name, exp_b[:7])}, - "branch": {(ref_info_c.name, exp_c[:7])}, + baseline_a: {(ref_info_a.name, exp_a), (ref_info_b.name, exp_b)}, + "branch": {(ref_info_c.name, exp_c)}, } diff --git a/tests/func/experiments/test_remote.py b/tests/func/experiments/test_remote.py index 9b8a95999f..d2768aef75 100644 --- a/tests/func/experiments/test_remote.py +++ b/tests/func/experiments/test_remote.py @@ -179,12 +179,12 @@ def test_list_remote(tmp_dir, scm, dvc, git_downstream, exp_stage, use_url): git_downstream.tmp_dir.scm.fetch_refspecs(remote, ["master:master"]) exp_list = downstream_exp.ls(rev=baseline_a, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, None), (ref_info_b.name, None)} + baseline_a: {(ref_info_a.name, None), (ref_info_b.name, None)} } exp_list = downstream_exp.ls(all_commits=True, git_remote=remote) assert {key: set(val) for key, val in exp_list.items()} == { - baseline_a[:7]: {(ref_info_a.name, None), (ref_info_b.name, None)}, + baseline_a: {(ref_info_a.name, None), (ref_info_b.name, None)}, "master": {(ref_info_c.name, None)}, } diff --git a/tests/func/experiments/test_save.py b/tests/func/experiments/test_save.py index 56394b2430..82231cd155 100644 --- a/tests/func/experiments/test_save.py +++ b/tests/func/experiments/test_save.py @@ -70,7 +70,7 @@ def test_exp_save_after_commit(tmp_dir, dvc, scm): dvc.experiments.save(name="exp-2", force=True) all_exps = dvc.experiments.ls(all_commits=True) - assert all_exps[baseline[:7]][0][0] == "exp-1" + assert all_exps[baseline][0][0] == "exp-1" assert all_exps["master"][0][0] == "exp-2"