From 470f411cd6a92835968779a0925b5a6b4b8608cb Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Fri, 20 Nov 2020 17:08:05 +0545 Subject: [PATCH] parametrization: improve set error message (#4923) --- dvc/parsing/__init__.py | 32 +++++++++++++++++++++----- dvc/parsing/context.py | 30 +++++++++++++++++++++---- tests/unit/test_context.py | 21 +++++++++++++++--- tests/unit/test_stage_resolver.py | 37 +++++++++++++++++++++---------- 4 files changed, 95 insertions(+), 25 deletions(-) diff --git a/dvc/parsing/__init__.py b/dvc/parsing/__init__.py index 75c549c30..011dcdfcc 100644 --- a/dvc/parsing/__init__.py +++ b/dvc/parsing/__init__.py @@ -9,9 +9,10 @@ from typing import TYPE_CHECKING, List, Set from funcy import join from dvc.dependency.param import ParamsDependency +from dvc.exceptions import DvcException from dvc.path_info import PathInfo -from .context import Context +from .context import Context, SetError if TYPE_CHECKING: from dvc.repo import Repo @@ -30,6 +31,17 @@ SET_KWD = "set" DEFAULT_SENTINEL = object() +class ResolveError(DvcException): + pass + + +def _reraise_err(exc_cls, *args, from_exc=None): + err = exc_cls(*args) + if from_exc and logger.isEnabledFor(logging.DEBUG): + raise err from from_exc + raise err + + class DataResolver: def __init__(self, repo: "Repo", wdir: PathInfo, d: dict): @@ -76,7 +88,9 @@ class DataResolver: def _resolve_entry(self, name: str, definition): context = Context.clone(self.global_ctx) if FOREACH_KWD in definition: - self.set_context_from(context, definition.get(SET_KWD, {})) + self.set_context_from( + context, definition.get(SET_KWD, {}), source=[name, "set"] + ) assert IN_KWD in definition return self._foreach( context, name, definition[FOREACH_KWD], definition[IN_KWD] @@ -91,7 +105,9 @@ class DataResolver: def _resolve_stage(self, context: Context, name: str, definition) -> dict: definition = deepcopy(definition) - self.set_context_from(context, definition.pop(SET_KWD, {})) + self.set_context_from( + context, definition.pop(SET_KWD, {}), source=[name, "set"] + ) wdir = self._resolve_wdir(context, definition.get(WDIR_KWD)) if self.wdir != wdir: logger.debug( @@ -153,6 +169,10 @@ class DataResolver: return join(gen) @classmethod - def set_context_from(cls, context: Context, to_set): - for key, value in to_set.items(): - context.set(key, value) + def set_context_from(cls, context: Context, to_set, source=None): + try: + for key, value in to_set.items(): + src_set = [*(source or []), key] + context.set(key, value, source=".".join(src_set)) + except SetError as exc: + _reraise_err(ResolveError, str(exc), from_exc=exc) diff --git a/dvc/parsing/context.py b/dvc/parsing/context.py index 3432eacf5..2815a8c68 100644 --- a/dvc/parsing/context.py +++ b/dvc/parsing/context.py @@ -11,6 +11,7 @@ from funcy import identity, rpartial from dvc.exceptions import DvcException from dvc.parsing.interpolate import ( + ParseError, get_expression, get_matches, is_exact_string, @@ -26,6 +27,10 @@ class ContextError(DvcException): pass +class SetError(ContextError): + pass + + class MergeError(ContextError): def __init__(self, key, new, into): self.key = key @@ -66,6 +71,7 @@ def _merge(into, update, overwrite): class Meta: source: Optional[str] = None dpaths: List[str] = field(default_factory=list) + local: bool = True @staticmethod def update_path(meta: "Meta", path: Union[str, int]): @@ -129,11 +135,17 @@ class Container(Node, ABC): def _convert(self, key, value): meta = Meta.update_path(self.meta, key) + return self._convert_with_meta(value, meta) + + @staticmethod + def _convert_with_meta(value, meta: Meta = None): if value is None or isinstance(value, PRIMITIVES): + assert meta return Value(value, meta=meta) elif isinstance(value, Node): return value elif isinstance(value, (list, dict)): + assert meta container = CtxDict if isinstance(value, dict) else CtxList return container(value, meta=meta) else: @@ -255,6 +267,9 @@ class Context(CtxDict): if not self._track: return + if node.meta and node.meta.local: + return + for source, keys in node.get_sources().items(): if not source: continue @@ -294,7 +309,7 @@ class Context(CtxDict): _, ext = os.path.splitext(file) loader = LOADERS[ext] - meta = Meta(source=file) + meta = Meta(source=file, local=False) return cls(loader(file, tree=tree), meta=meta) def merge_from(self, tree, path, overwrite=False): @@ -310,7 +325,7 @@ class Context(CtxDict): """Clones given context.""" return cls(deepcopy(ctx.data)) - def set(self, key, value): + def set(self, key, value, source=None): """ Sets a value, either non-interpolated values to a key, or an interpolated string after resolving it. @@ -320,6 +335,13 @@ class Context(CtxDict): >>> c {'foo': 'foo', 'bar': [1, 2], 'lorem': {'a': 'z'}, 'foobar': [1, 2]} """ + try: + self._set(key, value, source) + except (ParseError, ValueError, ContextError) as exc: + sp = "\n" if isinstance(exc, ParseError) else " " + raise SetError(f"Failed to set '{source}':{sp}{str(exc)}") from exc + + def _set(self, key, value, source): if key in self: raise ValueError(f"Cannot set '{key}', key already exists") if isinstance(value, str): @@ -328,7 +350,7 @@ class Context(CtxDict): elif isinstance(value, (Sequence, Mapping)): self._check_not_nested_collection(key, value) self._check_interpolation_collection(key, value) - self[key] = value + self[key] = self._convert_with_meta(value, Meta(source=source)) def resolve(self, src, unwrap=True): """Recursively resolves interpolation and returns resolved data. @@ -399,7 +421,7 @@ class Context(CtxDict): if matches and not is_exact_string(value, matches): raise ValueError( f"Cannot set '{key}', " - "joining string with interpolated string" + "joining string with interpolated string " "is not supported" ) diff --git a/tests/unit/test_context.py b/tests/unit/test_context.py index 777f59d10..d2cacaa30 100644 --- a/tests/unit/test_context.py +++ b/tests/unit/test_context.py @@ -221,20 +221,35 @@ def test_load_from(mocker): file = "params.yaml" c = Context.load_from(object(), file) - assert asdict(c["x"].meta) == {"source": file, "dpaths": ["x"]} - assert asdict(c["foo"].meta) == {"source": file, "dpaths": ["foo"]} - assert asdict(c["x"]["y"].meta) == {"source": file, "dpaths": ["x", "y"]} + assert asdict(c["x"].meta) == { + "source": file, + "dpaths": ["x"], + "local": False, + } + assert asdict(c["foo"].meta) == { + "source": file, + "local": False, + "dpaths": ["foo"], + } + assert asdict(c["x"]["y"].meta) == { + "source": file, + "dpaths": ["x", "y"], + "local": False, + } assert asdict(c["x"]["y"]["z"].meta) == { "source": file, "dpaths": ["x", "y", "z"], + "local": False, } assert asdict(c["x"]["lst"].meta) == { "source": file, "dpaths": ["x", "lst"], + "local": False, } assert asdict(c["x"]["lst"][0].meta) == { "source": file, "dpaths": ["x", "lst", "0"], + "local": False, } diff --git a/tests/unit/test_stage_resolver.py b/tests/unit/test_stage_resolver.py index 14c50cc2e..619926b9e 100644 --- a/tests/unit/test_stage_resolver.py +++ b/tests/unit/test_stage_resolver.py @@ -2,7 +2,7 @@ from math import pi import pytest -from dvc.parsing import DataResolver +from dvc.parsing import DataResolver, ResolveError from dvc.parsing.context import Context, Value TEMPLATED_DVC_YAML_DATA = { @@ -75,17 +75,24 @@ def test_set(): ) def test_set_nested_coll(coll): context = Context(CONTEXT_DATA) - with pytest.raises(ValueError, match="Cannot set 'item', has nested"): + with pytest.raises(ResolveError) as exc_info: DataResolver.set_context_from(context, {"thresh": 10, "item": coll}) + assert ( + str(exc_info.value) == "Failed to set 'item': Cannot set 'item', " + "has nested dict/list" + ) + def test_set_already_exists(): context = Context({"item": "foo"}) - with pytest.raises( - ValueError, match="Cannot set 'item', key already exists" - ): + with pytest.raises(ResolveError) as exc_info: DataResolver.set_context_from(context, {"item": "bar"}) + assert ( + str(exc_info.value) == "Failed to set 'item': Cannot set 'item', " + "key already exists" + ) assert context["item"] == Value("foo") @@ -94,11 +101,15 @@ def test_set_already_exists(): ) def test_set_collection_interpolation(coll): context = Context(CONTEXT_DATA) - with pytest.raises( - ValueError, match="Cannot set 'item', having interpolation inside" - ): + with pytest.raises(ResolveError) as exc_info: DataResolver.set_context_from(context, {"thresh": 10, "item": coll}) + assert ( + str(exc_info.value) == "Failed to set 'item': Cannot set 'item', " + f"having interpolation inside '{type(coll).__name__}' " + "is not supported." + ) + def test_set_interpolated_string(): context = Context(CONTEXT_DATA) @@ -168,8 +179,10 @@ def test_set_ladder(): ) def test_set_multiple_interpolations(value): context = Context(CONTEXT_DATA) - with pytest.raises( - ValueError, - match=r"Cannot set 'item', joining string with interpolated string", - ): + with pytest.raises(ResolveError,) as exc_info: DataResolver.set_context_from(context, {"thresh": 10, "item": value}) + + assert str(exc_info.value) == ( + "Failed to set 'item': Cannot set 'item', " + "joining string with interpolated string is not supported" + )