Skip to content
/ dvc Public
forked from iterative/dvc

Commit

Permalink
params: fix skipping of params dvc.lock when it's a falsy value
Browse files Browse the repository at this point in the history
The falsy values were being skipped when merging params values
from dvc.yaml and dvc.lock, showing the values as always changed.

These values were never added to `info`, but were found in
`self.params` making them appear as changed.

Fixes iterative#4184
  • Loading branch information
skshetry committed Jul 9, 2020
1 parent 0899b27 commit aa70f35
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 9 deletions.
5 changes: 2 additions & 3 deletions dvc/dependency/param.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ def fill_values(self, values=None):
if not values:
return
for param in self.params:
value = values.get(param)
if value:
self.info[param] = value
if param in values:
self.info[param] = values[param]

def save(self):
super().save()
Expand Down
2 changes: 1 addition & 1 deletion tests/func/test_run_multistage.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ def test_run_params_default(tmp_dir, dvc):
params=["nested.nested1.nested2"],
cmd="cat params.yaml",
)
isinstance(stage.deps[0], ParamsDependency)
assert isinstance(stage.deps[0], ParamsDependency)
assert stage.deps[0].params == ["nested.nested1.nested2"]

lockfile = stage.dvcfile._lockfile
Expand Down
30 changes: 25 additions & 5 deletions tests/unit/dependency/test_params.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
from dvc.dependency import ParamsDependency, loadd_from, loads_params
from dvc.dependency.param import BadParamFileError, MissingParamsError
from dvc.stage import Stage
from dvc.utils.yaml import load_yaml

PARAMS = {
"foo": 1,
"bar": 53.135,
"baz": "str",
"qux": None,
}
DEFAULT_PARAMS_FILE = ParamsDependency.DEFAULT_PARAMS_FILE


def test_loads_params(dvc):
Expand Down Expand Up @@ -63,15 +65,15 @@ def test_loadd_from(dvc):
def test_dumpd_with_info(dvc):
dep = ParamsDependency(Stage(dvc), None, PARAMS)
assert dep.dumpd() == {
"path": "params.yaml",
"path": DEFAULT_PARAMS_FILE,
"params": PARAMS,
}


def test_dumpd_without_info(dvc):
dep = ParamsDependency(Stage(dvc), None, list(PARAMS.keys()))
assert dep.dumpd() == {
"path": "params.yaml",
"path": DEFAULT_PARAMS_FILE,
"params": list(PARAMS.keys()),
}

Expand All @@ -82,15 +84,16 @@ def test_read_params_nonexistent_file(dvc):


def test_read_params_unsupported_format(tmp_dir, dvc):
tmp_dir.gen("params.yaml", b"\0\1\2\3\4\5\6\7")
tmp_dir.gen(DEFAULT_PARAMS_FILE, b"\0\1\2\3\4\5\6\7")
dep = ParamsDependency(Stage(dvc), None, ["foo"])
with pytest.raises(BadParamFileError):
dep.read_params()


def test_read_params_nested(tmp_dir, dvc):
tmp_dir.gen(
"params.yaml", yaml.dump({"some": {"path": {"foo": ["val1", "val2"]}}})
DEFAULT_PARAMS_FILE,
yaml.dump({"some": {"path": {"foo": ["val1", "val2"]}}}),
)
dep = ParamsDependency(Stage(dvc), None, ["some.path.foo"])
assert dep.read_params() == {"some.path.foo": ["val1", "val2"]}
Expand All @@ -103,7 +106,24 @@ def test_save_info_missing_config(dvc):


def test_save_info_missing_param(tmp_dir, dvc):
tmp_dir.gen("params.yaml", "bar: baz")
tmp_dir.gen(DEFAULT_PARAMS_FILE, "bar: baz")
dep = ParamsDependency(Stage(dvc), None, ["foo"])
with pytest.raises(MissingParamsError):
dep.save_info()


@pytest.mark.parametrize(
"param_value",
["", "false", "[]", "{}", "null", "no", "off"]
# we use pyyaml to load params.yaml, which only supports YAML 1.1
# so, some of the above are boolean values
)
def test_params_with_false_values(tmp_dir, dvc, param_value):
key = "param"
dep = ParamsDependency(Stage(dvc), DEFAULT_PARAMS_FILE, [key])
(tmp_dir / DEFAULT_PARAMS_FILE).write_text(f"{key}: {param_value}")

dep.fill_values(load_yaml(DEFAULT_PARAMS_FILE))

with dvc.state:
assert dep.status() == {}

0 comments on commit aa70f35

Please sign in to comment.