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.

Sign up for GitHub

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

Jump to bottom

exp show: show metrics that include separator #9819

Merged
merged 7 commits into from
Aug 24, 2023
Merged

exp show: show metrics that include separator #9819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
32a6974
exp show: show metrics that include separator
Aug 8, 2023
d91e08c
sort using partition instead of lpartition
Aug 8, 2023
fff14b6
handle ':' in file and metric names
Aug 22, 2023
2e7a254
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 22, 2023
25ab060
fix sorting with leading :
Aug 23, 2023
d4600e9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 23, 2023
b12f501
Merge branch 'main' into metric-name-colon
dberenbaum Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions dvc/repo/experiments/show.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,26 +177,31 @@ def _build_rows(
)


def _sort_column(
def _sort_column( # noqa: C901
sort_by: str,
metric_names: Mapping[str, Iterable[str]],
param_names: Mapping[str, Iterable[str]],
) -> Tuple[str, str, str]:
path, _, sort_name = sort_by.rpartition(":")
sep = ":"
parts = sort_by.split(sep)
matches: Set[Tuple[str, str, str]] = set()

if path:
for split_num in range(len(parts)):
path = sep.join(parts[:split_num])
sort_name = sep.join(parts[split_num:])
if not path: # handles ':metric_name' case
sort_by = sort_name
if path in metric_names and sort_name in metric_names[path]:
matches.add((path, sort_name, "metrics"))
if path in param_names and sort_name in param_names[path]:
matches.add((path, sort_name, "params"))
else:
if not matches:
for path in metric_names:
if sort_name in metric_names[path]:
matches.add((path, sort_name, "metrics"))
if sort_by in metric_names[path]:
matches.add((path, sort_by, "metrics"))
for path in param_names:
if sort_name in param_names[path]:
matches.add((path, sort_name, "params"))
if sort_by in param_names[path]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support it in --set-param or are we implying that this will only be supported for exp show / metrics?

(in the current fix we are also supporting params in the exp show)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth discussing, but I figured even this limited fix is better than the current situation.

matches.add((path, sort_by, "params"))

if len(matches) == 1:
return matches.pop()
Expand Down
14 changes: 14 additions & 0 deletions tests/func/experiments/test_show.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,20 @@ def test_show_sort(tmp_dir, scm, dvc, exp_stage, caplog):
assert main(["exp", "show", "--no-pager", "--sort-by=metrics.yaml:foo"]) == 0


def test_show_sort_metric_sep(tmp_dir, scm, dvc, caplog):
daavoo marked this conversation as resolved.
Show resolved Hide resolved
metrics_path = tmp_dir / "metrics:1.json"
metrics_path.write_text('{"my::metric": 1, "other_metric": 0.5}')
metrics_path = tmp_dir / "metrics:2.json"
metrics_path.write_text('{"my::metric": 2}')
dvcyaml_path = tmp_dir / "dvc.yaml"
dvcyaml_path.write_text("metrics: ['metrics:1.json', 'metrics:2.json']")
dvc.experiments.save()
assert (
main(["exp", "show", "--no-pager", "--sort-by=metrics:1.json:my::metric"]) == 0
)
assert main(["exp", "show", "--no-pager", "--sort-by=:other_metric"]) == 0


@pytest.mark.vscode
@pytest.mark.parametrize(
"status, pid_exists",
Expand Down
Loading