diff --git a/dvc/repo/experiments/__init__.py b/dvc/repo/experiments/__init__.py index e3fefc605..30b9a5d55 100644 --- a/dvc/repo/experiments/__init__.py +++ b/dvc/repo/experiments/__init__.py @@ -10,6 +10,7 @@ from typing import Dict, Iterable, Mapping, Optional from funcy import cached_property, first +from dvc.dependency.param import MissingParamsError from dvc.env import DVCLIVE_RESUME from dvc.exceptions import DvcException from dvc.path_info import PathInfo @@ -350,9 +351,19 @@ class Experiments: ) self.scm.add(self.args_file) + def _format_new_params_msg(self, new_params, config_path): + """Format an error message for when new parameters are identified""" + new_param_count = len(new_params) + pluralise = "s are" if new_param_count > 1 else " is" + param_list = ", ".join(new_params) + return ( + f"{new_param_count} parameter{pluralise} missing " + f"from '{config_path}': {param_list}" + ) + def _update_params(self, params: dict): """Update experiment params files with the specified values.""" - from dvc.utils.collections import merge_params + from dvc.utils.collections import NewParamsFound, merge_params from dvc.utils.serialize import MODIFIERS logger.debug("Using experiment params '%s'", params) @@ -362,7 +373,11 @@ class Experiments: suffix = path.suffix.lower() modify_data = MODIFIERS[suffix] with modify_data(path, fs=self.repo.fs) as data: - merge_params(data, params[params_fname]) + try: + merge_params(data, params[params_fname], allow_new=False) + except NewParamsFound as e: + msg = self._format_new_params_msg(e.new_params, path) + raise MissingParamsError(msg) # Force params file changes to be staged in git # Otherwise in certain situations the changes to params file may be diff --git a/dvc/utils/collections.py b/dvc/utils/collections.py index f05cdb8f5..1b62cf743 100644 --- a/dvc/utils/collections.py +++ b/dvc/utils/collections.py @@ -6,6 +6,16 @@ from typing import Callable, Dict, Iterable, List, TypeVar, Union from pygtrie import StringTrie as _StringTrie +from dvc.exceptions import DvcException + + +class NewParamsFound(DvcException): + """Thrown if new params were found during merge_params""" + + def __init__(self, new_params: List, *args): + self.new_params = new_params + super().__init__("New params found during merge", *args) + class PathStringTrie(_StringTrie): """Trie based on platform-dependent separator for pathname components.""" @@ -80,11 +90,27 @@ def chunk_dict(d: Dict[_KT, _VT], size: int = 1) -> List[Dict[_KT, _VT]]: return [{key: d[key] for key in chunk} for chunk in chunks(size, d)] -def merge_params(src: Dict, to_update: Dict) -> Dict: - """Recursively merges params with benedict's syntax support in-place.""" +def merge_params(src: Dict, to_update: Dict, allow_new: bool = True) -> Dict: + """ + Recursively merges params with benedict's syntax support in-place. + + Args: + src (dict): source dictionary of parameters + to_update (dict): dictionary of parameters to merge into src + allow_new (bool): if False, raises an error if new keys would be + added to src + """ from ._benedict import benedict data = benedict(src) + + if not allow_new: + new_params = list( + set(to_update.keys()) - set(data.keypaths(indexes=True)) + ) + if new_params: + raise NewParamsFound(new_params) + data.merge(to_update, overwrite=True) return src diff --git a/tests/func/experiments/test_experiments.py b/tests/func/experiments/test_experiments.py index a06cedf2e..cf94871f5 100644 --- a/tests/func/experiments/test_experiments.py +++ b/tests/func/experiments/test_experiments.py @@ -7,6 +7,7 @@ import pytest from funcy import first from dvc.dvcfile import PIPELINE_FILE +from dvc.exceptions import DvcException from dvc.repo.experiments.utils import exp_refs_by_rev from dvc.utils.serialize import PythonFileCorruptedError from tests.func.test_repro_multistage import COPY_SCRIPT @@ -118,10 +119,6 @@ def test_failed_exp(tmp_dir, scm, dvc, exp_stage, mocker, caplog): ["foo[1]=[baz, goo]"], "{foo: [bar: 1, [baz, goo]], goo: {bag: 3}, lorem: false}", ], - [ - ["lorem.ipsum=3"], - "{foo: [bar: 1, baz: 2], goo: {bag: 3}, lorem: {ipsum: 3}}", - ], ], ) def test_modify_params(tmp_dir, scm, dvc, mocker, changes, expected): @@ -148,6 +145,28 @@ def test_modify_params(tmp_dir, scm, dvc, mocker, changes, expected): assert fobj.read().strip() == expected +@pytest.mark.parametrize( + "changes", + [["lorem.ipsum=3"], ["foo[0].bazar=3"]], +) +def test_add_params(tmp_dir, scm, dvc, changes): + tmp_dir.gen("copy.py", COPY_SCRIPT) + tmp_dir.gen( + "params.yaml", "{foo: [bar: 1, baz: 2], goo: {bag: 3}, lorem: false}" + ) + stage = dvc.run( + cmd="python copy.py params.yaml metrics.yaml", + metrics_no_cache=["metrics.yaml"], + params=["foo", "goo", "lorem"], + name="copy-file", + ) + scm.add(["dvc.yaml", "dvc.lock", "copy.py", "params.yaml", "metrics.yaml"]) + scm.commit("init") + + with pytest.raises(DvcException): + dvc.experiments.run(stage.addressing, params=changes) + + @pytest.mark.parametrize("queue", [True, False]) def test_apply(tmp_dir, scm, dvc, exp_stage, queue): from dvc.exceptions import InvalidArgumentError @@ -387,7 +406,7 @@ def test_no_scm(tmp_dir): @pytest.mark.parametrize("workspace", [True, False]) def test_untracked(tmp_dir, scm, dvc, caplog, workspace): tmp_dir.gen("copy.py", COPY_SCRIPT) - tmp_dir.gen("params.yaml", "foo: 1") + tmp_dir.scm_gen("params.yaml", "foo: 1", commit="track params") stage = dvc.run( cmd="python copy.py params.yaml metrics.yaml", metrics_no_cache=["metrics.yaml"],