From 43953ace5305eae9364fab3ec709718700c49ece Mon Sep 17 00:00:00 2001 From: David de la Iglesia Castro Date: Mon, 3 Jan 2022 20:53:21 +0100 Subject: [PATCH] `exp show`: Add `--drop` and `--keep` args Removed `--include-metrics` / `--include-params` and `--exclude-metrics` / `--exclude-params`. Removed `--no-timestamp` . Can be done by `--drop Created`. `--drop` and `--keep` operate directly on the table columns. `--keep` does not perform any filtering. It's only used to specify columns to keep despite the other filtering. For example `--only-changed --keep foo` will prevent `foo` from being removed by `--only-changed`. Another example, `--drop train.* --keep train.dropout` will remove all columns matching `train.*` except for `train.dropout`. Closes #7079 Closes #7080 Pre-requisite for #6434 --- dvc/commands/experiments/show.py | 180 ++++++------------------- tests/func/experiments/test_show.py | 174 +++++++----------------- tests/unit/command/test_experiments.py | 4 +- 3 files changed, 91 insertions(+), 267 deletions(-) diff --git a/dvc/commands/experiments/show.py b/dvc/commands/experiments/show.py index e13383c13..ead3ed542 100644 --- a/dvc/commands/experiments/show.py +++ b/dvc/commands/experiments/show.py @@ -1,10 +1,10 @@ import argparse import logging import os +import re from collections import Counter, OrderedDict, defaultdict from datetime import date, datetime -from fnmatch import fnmatch -from typing import TYPE_CHECKING, Dict, Iterable, Optional +from typing import TYPE_CHECKING from funcy import lmap @@ -28,64 +28,6 @@ FILL_VALUE_ERRORED = "!" logger = logging.getLogger(__name__) -def _filter_name(names, label, filter_strs): - ret = defaultdict(dict) - path_filters = defaultdict(list) - - for filter_s in filter_strs: - path, _, name = filter_s.rpartition(":") - path_filters[path].append(name) - - for path, filters in path_filters.items(): - if path: - match_paths = [path] - else: - match_paths = names.keys() - for match_path in match_paths: - for f in filters: - matches = [ - name for name in names[match_path] if fnmatch(name, f) - ] - if not matches: - raise InvalidArgumentError( - f"'{f}' does not match any known {label}" - ) - ret[match_path].update({match: None for match in matches}) - - return ret - - -def _filter_names( - names: Dict[str, Dict[str, None]], - label: str, - include: Optional[Iterable], - exclude: Optional[Iterable], -): - if include and exclude: - intersection = set(include) & set(exclude) - if intersection: - values = ", ".join(intersection) - raise InvalidArgumentError( - f"'{values}' specified in both --include-{label} and" - f" --exclude-{label}" - ) - - if include: - ret = _filter_name(names, label, include) - else: - ret = names - - if exclude: - to_remove = _filter_name(names, label, exclude) - for path in to_remove: - if path in ret: - for key in to_remove[path]: - if key in ret[path]: - del ret[path][key] - - return ret - - def _update_names(names, items): for name, item in items: item = item.get("data", {}) @@ -103,18 +45,6 @@ def _collect_names(all_experiments, **kwargs): exp = exp_data.get("data", {}) _update_names(metric_names, exp.get("metrics", {}).items()) _update_names(param_names, exp.get("params", {}).items()) - metric_names = _filter_names( - metric_names, - "metrics", - kwargs.get("include_metrics"), - kwargs.get("exclude_metrics"), - ) - param_names = _filter_names( - param_names, - "params", - kwargs.get("include_params"), - kwargs.get("exclude_params"), - ) return metric_names, param_names @@ -308,17 +238,6 @@ def _extend_row(row, names, items, precision, fill_value=FILL_VALUE): row.append(ui.rich_text(str(_format_field(value, precision)))) -def _parse_filter_list(param_list): - ret = [] - for param_str in param_list: - path, _, param_str = param_str.rpartition(":") - if path: - ret.extend(f"{path}:{param}" for param in param_str.split(",")) - else: - ret.extend(param_str.split(",")) - return ret - - def experiments_table( all_experiments, headers, @@ -381,8 +300,9 @@ def baseline_styler(typ): def show_experiments( all_experiments, + keep=None, + drop=None, pager=True, - no_timestamp=False, csv=False, markdown=False, pcp=False, @@ -390,18 +310,7 @@ def show_experiments( ): from funcy.seqs import flatten as flatten_list - include_metrics = _parse_filter_list(kwargs.pop("include_metrics", [])) - exclude_metrics = _parse_filter_list(kwargs.pop("exclude_metrics", [])) - include_params = _parse_filter_list(kwargs.pop("include_params", [])) - exclude_params = _parse_filter_list(kwargs.pop("exclude_params", [])) - - metric_names, param_names = _collect_names( - all_experiments, - include_metrics=include_metrics, - exclude_metrics=exclude_metrics, - include_params=include_params, - exclude_params=exclude_params, - ) + metric_names, param_names = _collect_names(all_experiments) headers = [ "Experiment", @@ -432,9 +341,10 @@ def show_experiments( kwargs.get("fill_value"), kwargs.get("iso"), ) - - if no_timestamp or pcp: - td.drop("Created") + if keep: + for col in td.keys(): + if re.match(keep, col): + td.protect(col) for col in ("State", "Executor"): if td.is_empty(col): @@ -473,8 +383,21 @@ def show_experiments( if kwargs.get("only_changed", False) or pcp: td.drop_duplicates("cols", ignore_empty=False) + cols_to_drop = set() + if drop is not None: + cols_to_drop = {col for col in td.keys() if re.match(drop, col)} if pcp: - td.dropna("rows", how="all") + cols_to_drop.add("Created") + td.drop(*cols_to_drop) + + if pcp: + subset = [x for x in td.keys() if x != "Experiment"] + td.dropna( + "rows", + how="all", + subset=subset, + ) + td.drop_duplicates("rows", subset=subset) td.column("Experiment")[:] = [ # remove tree characters str(x).encode("ascii", "ignore").strip().decode() @@ -545,11 +468,8 @@ class CmdExperimentsShow(CmdBase): show_experiments( all_experiments, - include_metrics=self.args.include_metrics, - exclude_metrics=self.args.exclude_metrics, - include_params=self.args.include_params, - exclude_params=self.args.exclude_params, - no_timestamp=self.args.no_timestamp, + keep=self.args.keep, + drop=self.args.drop, sort_by=self.args.sort_by, sort_order=self.args.sort_order, precision=precision, @@ -622,32 +542,23 @@ def add_parser(experiments_subparsers, parent_parser): help="Do not pipe output into a pager.", ) experiments_show_parser.add_argument( - "--include-metrics", - action="append", - default=[], - help="Include the specified metrics in output table.", - metavar="", + "--only-changed", + action="store_true", + default=False, + help=( + "Only show metrics/params with values varying " + "across the selected experiments." + ), ) experiments_show_parser.add_argument( - "--exclude-metrics", - action="append", - default=[], - help="Exclude the specified metrics from output table.", - metavar="", + "--drop", + help="Remove the columns matching the specified regex pattern.", + metavar="", ) experiments_show_parser.add_argument( - "--include-params", - action="append", - default=[], - help="Include the specified params in output table.", - metavar="", - ) - experiments_show_parser.add_argument( - "--exclude-params", - action="append", - default=[], - help="Exclude the specified params from output table.", - metavar="", + "--keep", + help="Preserve the columns matching the specified regex pattern.", + metavar="", ) experiments_show_parser.add_argument( "--param-deps", @@ -669,12 +580,6 @@ def add_parser(experiments_subparsers, parent_parser): choices=("asc", "desc"), default="asc", ) - experiments_show_parser.add_argument( - "--no-timestamp", - action="store_true", - default=False, - help="Do not show experiment timestamps.", - ) experiments_show_parser.add_argument( "--sha", action="store_true", @@ -712,15 +617,6 @@ def add_parser(experiments_subparsers, parent_parser): ), metavar="", ) - experiments_show_parser.add_argument( - "--only-changed", - action="store_true", - default=False, - help=( - "Only show metrics/params with values varying " - "across the selected experiments." - ), - ) experiments_show_parser.add_argument( "--parallel-coordinates-plot", "--pcp", diff --git a/tests/func/experiments/test_show.py b/tests/func/experiments/test_show.py index 8fe2b87a6..2b69ea07d 100644 --- a/tests/func/experiments/test_show.py +++ b/tests/func/experiments/test_show.py @@ -18,7 +18,6 @@ from dvc.repo.experiments.utils import exp_refs_by_rev from dvc.utils.fs import makedirs from dvc.utils.serialize import YAMLFileCorruptedError from tests.func.test_repro_multistage import COPY_SCRIPT -from tests.utils import console_width def make_executor_info(**kwargs): @@ -193,105 +192,12 @@ def test_show_checkpoint_branch( assert f"({branch_rev[:7]})" in cap.out -@pytest.mark.parametrize( - "i_metrics,i_params,e_metrics,e_params,included,excluded", - [ - ( - "foo", - "foo", - None, - None, - ["foo"], - ["bar", "train/foo", "nested.foo"], - ), - ( - None, - None, - "foo", - "foo", - ["bar", "train/foo", "nested.foo"], - ["foo"], - ), - ( - "foo,bar", - "foo,bar", - None, - None, - ["foo", "bar"], - ["train/foo", "train/bar", "nested.foo", "nested.bar"], - ), - ( - "metrics.yaml:foo,bar", - "params.yaml:foo,bar", - None, - None, - ["foo", "bar"], - ["train/foo", "train/bar", "nested.foo", "nested.bar"], - ), - ( - "train/*", - "train/*", - None, - None, - ["train/foo", "train/bar"], - ["foo", "bar", "nested.foo", "nested.bar"], - ), - ( - None, - None, - "train/*", - "train/*", - ["foo", "bar", "nested.foo", "nested.bar"], - ["train/foo", "train/bar"], - ), - ( - "train/*", - "train/*", - "*foo", - "*foo", - ["train/bar"], - ["train/foo", "foo", "bar", "nested.foo", "nested.bar"], - ), - ( - "nested.*", - "nested.*", - None, - None, - ["nested.foo", "nested.bar"], - ["foo", "bar", "train/foo", "train/bar"], - ), - ( - None, - None, - "nested.*", - "nested.*", - ["foo", "bar", "train/foo", "train/bar"], - ["nested.foo", "nested.bar"], - ), - ( - "*.*", - "*.*", - "*.bar", - "*.bar", - ["nested.foo"], - ["foo", "bar", "nested.bar", "train/foo", "train/bar"], - ), - ], -) def test_show_filter( tmp_dir, scm, dvc, capsys, - i_metrics, - i_params, - e_metrics, - e_params, - included, - excluded, ): - from dvc.ui import ui - capsys.readouterr() tmp_dir.gen("copy.py", COPY_SCRIPT) @@ -324,26 +230,39 @@ def test_show_filter( ) scm.commit("init") - command = ["exp", "show", "--no-pager", "--no-timestamp"] - if i_metrics is not None: - command.append(f"--include-metrics={i_metrics}") - if i_params is not None: - command.append(f"--include-params={i_params}") - if e_metrics is not None: - command.append(f"--exclude-metrics={e_metrics}") - if e_params is not None: - command.append(f"--exclude-params={e_params}") - - with console_width(ui.rich_console, 255): - assert main(command) == 0 + capsys.readouterr() + assert main(["exp", "show", "--drop=.*foo"]) == 0 cap = capsys.readouterr() + for filtered in ["foo", "train/foo", "nested.foo"]: + assert f"params.yaml:{filtered}" not in cap.out + assert f"metrics.yaml:{filtered}" not in cap.out - for i in included: - assert f"params.yaml:{i}" in cap.out - assert f"metrics.yaml:{i}" in cap.out - for e in excluded: - assert f"params.yaml:{e}" not in cap.out - assert f"metrics.yaml:{e}" not in cap.out + capsys.readouterr() + assert main(["exp", "show", "--drop=.*foo", "--keep=.*train"]) == 0 + cap = capsys.readouterr() + for filtered in ["foo", "nested.foo"]: + assert f"params.yaml:{filtered}" not in cap.out + assert f"metrics.yaml:{filtered}" not in cap.out + assert "params.yaml:train/foo" in cap.out + assert "metrics.yaml:train/foo" in cap.out + + capsys.readouterr() + assert main(["exp", "show", "--drop=params.yaml:.*foo"]) == 0 + cap = capsys.readouterr() + for filtered in ["foo", "train/foo", "nested.foo"]: + assert f"params.yaml:{filtered}" not in cap.out + assert f"metrics.yaml:{filtered}" in cap.out + + capsys.readouterr() + assert main(["exp", "show", "--drop=Created"]) == 0 + cap = capsys.readouterr() + assert "Created" not in cap.out + + capsys.readouterr() + assert main(["exp", "show", "--drop=Created|Experiment"]) == 0 + cap = capsys.readouterr() + assert "Created" not in cap.out + assert "Experiment" not in cap.out def test_show_multiple_commits(tmp_dir, scm, dvc, exp_stage): @@ -589,17 +508,21 @@ def test_show_only_changed(tmp_dir, dvc, scm, capsys): capsys.readouterr() assert main(["exp", "show"]) == 0 cap = capsys.readouterr() - assert "bar" in cap.out capsys.readouterr() assert main(["exp", "show", "--only-changed"]) == 0 cap = capsys.readouterr() - assert "bar" not in cap.out + capsys.readouterr() + assert main(["exp", "show", "--only-changed", "--keep=.*bar"]) == 0 + cap = capsys.readouterr() + assert "params.yaml:bar" in cap.out + assert "metrics.yaml:bar" in cap.out -def test_show_parallel_coordinates(tmp_dir, dvc, scm, mocker): + +def test_show_parallel_coordinates(tmp_dir, dvc, scm, mocker, capsys): from dvc.commands.experiments import show webbroser_open = mocker.patch("webbrowser.open") @@ -638,22 +561,20 @@ def test_show_parallel_coordinates(tmp_dir, dvc, scm, mocker): kwargs = show_experiments.call_args[1] html_text = (tmp_dir / "dvc_plots" / "index.html").read_text() - assert all(rev in html_text for rev in ["workspace", "master", "[exp-"]) + assert all(rev in html_text for rev in ["workspace", "master"]) + assert "[exp-" not in html_text - assert ( - '{"label": "metrics.yaml:foo", "values": [2.0, 1.0, 2.0]}' in html_text - ) - assert ( - '{"label": "params.yaml:foo", "values": [2.0, 1.0, 2.0]}' in html_text - ) - assert '"line": {"color": [2, 1, 0]' in html_text + assert '{"label": "metrics.yaml:foo", "values": [2.0, 1.0]}' in html_text + assert '{"label": "params.yaml:foo", "values": [2.0, 1.0]}' in html_text + assert '"line": {"color": [1, 0]' in html_text assert '"label": "metrics.yaml:bar"' not in html_text + assert '"label": "Created"' not in html_text assert main(["exp", "show", "--pcp", "--sort-by", "metrics.yaml:foo"]) == 0 kwargs = show_experiments.call_args[1] html_text = (tmp_dir / "dvc_plots" / "index.html").read_text() - assert '"line": {"color": [2.0, 1.0, 2.0]' in html_text + assert '"line": {"color": [2.0, 1.0]' in html_text assert main(["exp", "show", "--pcp", "--out", "experiments"]) == 0 kwargs = show_experiments.call_args[1] @@ -670,3 +591,8 @@ def test_show_parallel_coordinates(tmp_dir, dvc, scm, mocker): assert main(["exp", "show", "--pcp"]) == 0 html_text = (tmp_dir / "dvc_plots" / "index.html").read_text() assert '{"label": "foobar", "values": [2.0, null, null]}' in html_text + + assert main(["exp", "show", "--pcp", "--drop", "foobar"]) == 0 + html_text = (tmp_dir / "dvc_plots" / "index.html").read_text() + assert '"label": "Created"' not in html_text + assert '"label": "foobar"' not in html_text diff --git a/tests/unit/command/test_experiments.py b/tests/unit/command/test_experiments.py index ae774f933..df3bb3feb 100644 --- a/tests/unit/command/test_experiments.py +++ b/tests/unit/command/test_experiments.py @@ -790,7 +790,9 @@ def test_show_experiments_pcp(tmp_dir, mocker): show_experiments(all_experiments, pcp=True) - td.dropna.assert_called_with("rows", how="all") + td.drop.assert_called_with("Created") + td.dropna.assert_called_with("rows", how="all", subset=[]) + td.drop_duplicates.assert_called_with("rows", subset=[]) kwargs = td.to_parallel_coordinates.call_args[1]