diff --git a/dvc/dependency/__init__.py b/dvc/dependency/__init__.py index 7e73d49ec..b21bdf25e 100644 --- a/dvc/dependency/__init__.py +++ b/dvc/dependency/__init__.py @@ -47,16 +47,29 @@ def loads_from(stage, s_list, erepo=None): def _merge_params(s_list): d = defaultdict(list) default_file = ParamsDependency.DEFAULT_PARAMS_FILE + + # figure out completely tracked params file, and ignore specific keys + wholly_tracked = set() + for key in s_list: + if not isinstance(key, dict): + continue + wholly_tracked.update(k for k, params in key.items() if not params) + for key in s_list: if isinstance(key, str): - d[default_file].append(key) + if default_file not in wholly_tracked: + d[default_file].append(key) continue + if not isinstance(key, dict): msg = "Only list of str/dict is supported. Got: " msg += f"'{type(key).__name__}'." raise ValueError(msg) for k, params in key.items(): + if k in wholly_tracked: + d[k] = [] + continue if not isinstance(params, list): msg = "Expected list of params for custom params file " msg += f"'{k}', got '{type(params).__name__}'." diff --git a/dvc/dependency/param.py b/dvc/dependency/param.py index f35a7e0b5..5bb979533 100644 --- a/dvc/dependency/param.py +++ b/dvc/dependency/param.py @@ -1,7 +1,9 @@ import errno import logging import os +import typing from collections import defaultdict +from typing import Dict import dpath.util from voluptuous import Any @@ -37,51 +39,82 @@ class ParamsDependency(Dependency): DEFAULT_PARAMS_FILE = "params.yaml" def __init__(self, stage, path, params=None, repo=None): - info = {} - self.params = params or [] - if params: - if isinstance(params, list): - self.params = params - else: - assert isinstance(params, dict) - self.params = list(params.keys()) - info = {self.PARAM_PARAMS: params} - - super().__init__( - stage, - path - or os.path.join(stage.repo.root_dir, self.DEFAULT_PARAMS_FILE), - info=info, - repo=repo, + self.params = list(params) if params else [] + info = ( + {self.PARAM_PARAMS: params} if isinstance(params, dict) else None ) + repo = repo or stage.repo + path = path or os.path.join(repo.root_dir, self.DEFAULT_PARAMS_FILE) + super().__init__(stage, path, info=info, repo=repo) def dumpd(self): ret = super().dumpd() if not self.hash_info: - ret[self.PARAM_PARAMS] = self.params + ret[self.PARAM_PARAMS] = self.params or {} return ret def fill_values(self, values=None): """Load params values dynamically.""" - if not values: + if values is None: return + info = {} + if not self.params: + info.update(values) for param in self.params: if param in values: info[param] = values[param] self.hash_info = HashInfo(self.PARAM_PARAMS, info) - def workspace_status(self): - status = super().workspace_status() + def read_params( + self, flatten: bool = True, **kwargs: typing.Any + ) -> Dict[str, typing.Any]: + try: + config = self.read_file() + except MissingParamsFile: + config = {} - if status.get(str(self)) == "deleted": - return status + if not self.params: + return config + + ret = {} + if flatten: + for param in self.params: + try: + ret[param] = dpath.util.get(config, param, separator=".") + except KeyError: + continue + return ret + + from dpath.util import merge + + for param in self.params: + merge( + ret, + dpath.util.search(config, param, separator="."), + separator=".", + ) + return ret + + def workspace_status(self): + if not self.exists: + return {str(self): "deleted"} + if self.hash_info.value is None: + return {str(self): "new"} + + from funcy import ldistinct status = defaultdict(dict) info = self.hash_info.value if self.hash_info else {} actual = self.read_params() - for param in self.params: - if param not in actual.keys(): + + # NOTE: we want to preserve the order of params as specified in the + # status. In case of tracking the whole file, the order is top-level + # keys in the file and then the keys in the `info` from `dvc.lock` + # (which are alphabetically sorted). + params = self.params or ldistinct([*actual.keys(), *info.keys()]) + for param in params: + if param not in actual: st = "deleted" elif param not in info: st = "new" @@ -130,35 +163,6 @@ class ParamsDependency(Dependency): f"Unable to read parameters from '{self}'" ) from exc - def _read(self): - try: - return self.read_file() - except MissingParamsFile: - return {} - - def read_params_d(self, **kwargs): - config = self._read() - - ret = {} - for param in self.params: - dpath.util.merge( - ret, - dpath.util.search(config, param, separator="."), - separator=".", - ) - return ret - - def read_params(self): - config = self._read() - - ret = {} - for param in self.params: - try: - ret[param] = dpath.util.get(config, param, separator=".") - except KeyError: - pass - return ret - def get_hash(self): info = self.read_params() @@ -179,12 +183,5 @@ class ParamsDependency(Dependency): if not self.isfile and not self.isdir: raise self.IsNotFileOrDirError(self) - if self.is_empty: - logger.warning(f"'{self}' is empty.") - self.ignore() - - if self.metric or self.plot: - self.verify_metric() - self.hash_info = self.get_hash() diff --git a/dvc/repo/params/show.py b/dvc/repo/params/show.py index 0a1d7bab3..701dec1ce 100644 --- a/dvc/repo/params/show.py +++ b/dvc/repo/params/show.py @@ -66,7 +66,9 @@ def _read_params( if deps: for param in params: - params_dict = error_handler(param.read_params_d)(onerror=onerror) + params_dict = error_handler(param.read_params)( + onerror=onerror, flatten=False + ) if params_dict: res[ repo.fs.path.relpath(param.fs_path, os.getcwd()) diff --git a/dvc/stage/serialize.py b/dvc/stage/serialize.py index bb6d556df..2a1e15b05 100644 --- a/dvc/stage/serialize.py +++ b/dvc/stage/serialize.py @@ -90,18 +90,12 @@ def _serialize_params_keys(params): """ keys = [] for param_dep in sort_by_path(params): - dump = param_dep.dumpd() - path, params = dump[PARAM_PATH], dump[PARAM_PARAMS] - assert isinstance(params, (dict, list)) # when on no_exec, params are not filled and are saved as list - k = sorted(params.keys() if isinstance(params, dict) else params) - if not k: - continue - - if path == DEFAULT_PARAMS_FILE: + k = sorted(param_dep.params) + if k and param_dep.def_path == DEFAULT_PARAMS_FILE: keys = k + keys else: - keys.append({path: k}) + keys.append({param_dep.def_path: k or None}) return keys diff --git a/tests/func/params/test_diff.py b/tests/func/params/test_diff.py index 836e46893..f409e4012 100644 --- a/tests/func/params/test_diff.py +++ b/tests/func/params/test_diff.py @@ -1,3 +1,5 @@ +import pytest + from dvc.utils import relpath @@ -218,3 +220,24 @@ def test_diff_targeted(tmp_dir, scm, dvc, run_copy): assert dvc.params.diff(a_rev="HEAD~2", targets=["other_params.yaml"]) == { "other_params.yaml": {"xyz": {"old": "val", "new": "val3"}} } + + +@pytest.mark.parametrize("file", ["params.yaml", "other_params.yaml"]) +def test_diff_without_targets_specified(tmp_dir, dvc, scm, file): + params_file = tmp_dir / file + params_file.dump({"foo": {"bar": "bar"}, "x": "0"}) + dvc.stage.add( + name="test", + cmd=f"echo {file}", + params=[{file: None}], + ) + scm.add_commit([params_file, "dvc.yaml"], message="foo") + + params_file.dump({"foo": {"bar": "baz"}, "y": "100"}) + assert dvc.params.diff() == { + file: { + "foo.bar": {"new": "baz", "old": "bar"}, + "x": {"new": None, "old": "0"}, + "y": {"new": "100", "old": None}, + } + } diff --git a/tests/func/params/test_show.py b/tests/func/params/test_show.py index d07cda0b4..b7ea742db 100644 --- a/tests/func/params/test_show.py +++ b/tests/func/params/test_show.py @@ -178,3 +178,17 @@ def test_log_errors(tmp_dir, scm, dvc, capsys, file, error_path): "DVC failed to load some parameters for following revisions: 'v1'." in error ) + + +@pytest.mark.parametrize("file", ["params.yaml", "other_params.yaml"]) +def test_show_without_targets_specified(tmp_dir, dvc, scm, file): + params_file = tmp_dir / file + data = {"foo": {"bar": "bar"}, "x": "0"} + params_file.dump(data) + dvc.stage.add( + name="test", + cmd=f"echo {file}", + params=[{file: None}], + ) + + assert dvc.params.show() == {"": {"data": {file: {"data": data}}}} diff --git a/tests/func/test_status.py b/tests/func/test_status.py index 27bdf0e58..a9f3cd30c 100644 --- a/tests/func/test_status.py +++ b/tests/func/test_status.py @@ -137,3 +137,49 @@ def test_status_outputs(tmp_dir, dvc): assert dvc.status(targets=["alice"]) == { "alice_bob": [{"changed outs": {"alice": "modified"}}] } + + +def test_params_without_targets(tmp_dir, dvc): + dvc.stage.add( + name="test", cmd="echo params.yaml", params=[{"params.yaml": None}] + ) + assert dvc.status() == { + "test": [{"changed deps": {"params.yaml": "deleted"}}] + } + + (tmp_dir / "params.yaml").touch() + assert dvc.status() == {"test": [{"changed deps": {"params.yaml": "new"}}]} + + dvc.commit("test", force=True) + # make sure that we are able to keep track of "empty" contents + # and be able to distinguish between no-lock-entry and empty-lock-entry. + assert (tmp_dir / "dvc.lock").parse() == { + "schema": "2.0", + "stages": { + "test": {"cmd": "echo params.yaml", "params": {"params.yaml": {}}} + }, + } + assert dvc.status() == {} + + (tmp_dir / "params.yaml").dump({"foo": "foo", "bar": "bar"}) + assert dvc.status() == { + "test": [ + {"changed deps": {"params.yaml": {"bar": "new", "foo": "new"}}} + ] + } + dvc.commit("test", force=True) + + (tmp_dir / "params.yaml").dump({"foo": "foobar", "lorem": "ipsum"}) + assert dvc.status() == { + "test": [ + { + "changed deps": { + "params.yaml": { + "bar": "deleted", + "foo": "modified", + "lorem": "new", + } + } + } + ] + } diff --git a/tests/unit/dependency/test_params.py b/tests/unit/dependency/test_params.py index 48e8fc2f1..d2a9b0a9f 100644 --- a/tests/unit/dependency/test_params.py +++ b/tests/unit/dependency/test_params.py @@ -40,6 +40,37 @@ def test_loads_params(dvc): assert not deps[2].hash_info +def test_loads_params_without_any_specific_targets(dvc): + stage = Stage(dvc) + deps = loads_params( + stage, + [ + "foo", + {"params.yaml": None}, + {"a_file": []}, + {"b_file": ["baz"]}, + {"b_file": ["bat"]}, + {"a_file": ["foobar"]}, + ], + ) + assert len(deps) == 3 + + assert isinstance(deps[0], ParamsDependency) + assert deps[0].def_path == ParamsDependency.DEFAULT_PARAMS_FILE + assert deps[0].params == [] + assert not deps[0].hash_info + + assert isinstance(deps[1], ParamsDependency) + assert deps[1].def_path == "a_file" + assert deps[1].params == [] + assert not deps[1].hash_info + + assert isinstance(deps[2], ParamsDependency) + assert deps[2].def_path == "b_file" + assert deps[2].params == ["baz", "bat"] + assert not deps[2].hash_info + + @pytest.mark.parametrize("params", [[3], [{"b_file": "cat"}]]) def test_params_error(dvc, params): with pytest.raises(ValueError): @@ -199,3 +230,25 @@ def test_params_with_false_values(tmp_dir, dvc, param_value): dep.fill_values(load_yaml(DEFAULT_PARAMS_FILE)) assert dep.status() == {} + + +def test_params_status_without_targets(tmp_dir, dvc): + params_file = tmp_dir / "params.yaml" + dep = ParamsDependency(Stage(dvc), str(params_file), []) + + assert dep.hash_info.value is None + assert dep.status() == {"params.yaml": "deleted"} + + params_file.dump({"foo": "foo", "bar": "bar"}) + + assert dep.status() == {"params.yaml": "new"} + + dep.fill_values({}) + assert dep.hash_info.value == {} + assert dep.status() == {"params.yaml": {"bar": "new", "foo": "new"}} + + dep.fill_values({"foo": "foobar", "lorem": "ipsum"}) + assert dep.hash_info.value == {"foo": "foobar", "lorem": "ipsum"} + assert dep.status() == { + "params.yaml": {"bar": "new", "foo": "modified", "lorem": "deleted"} + } diff --git a/tests/unit/stage/test_serialize_pipeline_file.py b/tests/unit/stage/test_serialize_pipeline_file.py index 113591bd0..f0e97abfc 100644 --- a/tests/unit/stage/test_serialize_pipeline_file.py +++ b/tests/unit/stage/test_serialize_pipeline_file.py @@ -100,6 +100,25 @@ def test_params_file_sorted(dvc): ] +def test_params_file_without_targets(dvc): + params = [ + "foo", + "bar", + {"params.yaml": None}, + {"custom.yaml": ["wxyz", "pqrs", "baz"]}, + {"a-file-of-params.yaml": None}, + {"a-file-of-params.yaml": ["barr"]}, + ] + stage = create_stage( + PipelineStage, dvc, outs=["bar"], deps=["foo"], params=params, **kwargs + ) + assert to_pipeline_file(stage)["something"][stage.PARAM_PARAMS] == [ + {"a-file-of-params.yaml": None}, + {"custom.yaml": ["baz", "pqrs", "wxyz"]}, + {"params.yaml": None}, + ] + + @pytest.mark.parametrize( "typ, extra", [("plots", {"plot": True}), ("metrics", {"metric": True}), ("outs", {})], diff --git a/tests/unit/stage/test_serialize_pipeline_lock.py b/tests/unit/stage/test_serialize_pipeline_lock.py index 968b3183f..6add781cb 100644 --- a/tests/unit/stage/test_serialize_pipeline_lock.py +++ b/tests/unit/stage/test_serialize_pipeline_lock.py @@ -120,6 +120,25 @@ def test_lock_params_no_values_filled(dvc): assert to_single_stage_lockfile(stage) == {"cmd": "command"} +@pytest.mark.parametrize( + "info, expected", + [ + (None, {}), + ({}, {}), + ({"foo": "foo", "bar": "bar"}, {"bar": "bar", "foo": "foo"}), + ], +) +def test_lock_params_without_targets(dvc, info, expected): + stage = create_stage( + PipelineStage, dvc, params=[{"params.yaml": None}], **kwargs + ) + stage.deps[0].fill_values(info) + assert to_single_stage_lockfile(stage) == { + "cmd": "command", + "params": {"params.yaml": OrderedDict(expected)}, + } + + @pytest.mark.parametrize("typ", ["plots", "metrics", "outs"]) def test_lock_outs(dvc, typ): stage = create_stage(PipelineStage, dvc, **{typ: ["input"]}, **kwargs)