parametrization: improve 'vars' error message (#4924)

* Catches MergeError
* Catches params file not existing error

dvc/parsing/__init__.py

@@ -10,9 +10,11 @@ from funcy import join
 
 from dvc.dependency.param import ParamsDependency
 from dvc.exceptions import DvcException
+from dvc.parsing.interpolate import ParseError
 from dvc.path_info import PathInfo
+from dvc.utils import relpath
 
-from .context import Context, SetError
+from .context import Context, MergeError, Meta, ParamsFileNotFound, SetError
 
 if TYPE_CHECKING:
     from dvc.repo import Repo
@@ -35,6 +37,15 @@ class ResolveError(DvcException):
     pass
 
 
+def format_and_raise(exc, msg, path):
+    spacing = "\n" if isinstance(exc, (ParseError, MergeError)) else " "
+    message = f"failed to parse {msg} in '{path}':{spacing}{str(exc)}"
+
+    # FIXME: cannot reraise because of how we log "cause" of the exception
+    # the error message is verbose, hence need control over the spacing
+    _reraise_err(ResolveError, message, from_exc=exc)
+
+
 def _reraise_err(exc_cls, *args, from_exc=None):
     err = exc_cls(*args)
     if from_exc and logger.isEnabledFor(logging.DEBUG):
@@ -44,16 +55,17 @@ def _reraise_err(exc_cls, *args, from_exc=None):
 
 class DataResolver:
     def __init__(self, repo: "Repo", wdir: PathInfo, d: dict):
-
         self.data: dict = d
         self.wdir = wdir
         self.repo = repo
+        self.tree = self.repo.tree
         self.imported_files: Set[str] = set()
+        self.relpath = relpath(self.wdir / "dvc.yaml")
 
         to_import: PathInfo = wdir / DEFAULT_PARAMS_FILE
-        if repo.tree.exists(to_import):
+        if self.tree.exists(to_import):
             self.imported_files = {os.path.abspath(to_import)}
-            self.global_ctx = Context.load_from(repo.tree, str(to_import))
+            self.global_ctx = Context.load_from(self.tree, to_import)
         else:
             self.global_ctx = Context()
             logger.debug(
@@ -62,9 +74,12 @@ class DataResolver:
             )
 
         vars_ = d.get(VARS_KWD, [])
-        self.load_from_vars(
-            self.global_ctx, vars_, wdir, skip_imports=self.imported_files
-        )
+        try:
+            self.load_from_vars(
+                self.global_ctx, vars_, wdir, skip_imports=self.imported_files
+            )
+        except (ParamsFileNotFound, MergeError) as exc:
+            format_and_raise(exc, "'vars'", self.relpath)
 
     def load_from_vars(
         self,
@@ -72,18 +87,23 @@ class DataResolver:
         vars_: List,
         wdir: PathInfo,
         skip_imports: Set[str],
+        stage_name: str = None,
     ):
-        for item in vars_:
+        stage_name = stage_name or ""
+        for index, item in enumerate(vars_):
             assert isinstance(item, (str, dict))
             if isinstance(item, str):
-                path = os.path.abspath(wdir / item)
+                path_info = wdir / item
+                path = os.path.abspath(path_info)
                 if path in skip_imports:
                     continue
 
-                context.merge_from(self.repo.tree, str(path))
+                context.merge_from(self.tree, path_info)
                 skip_imports.add(path)
             else:
-                context.merge_update(Context(item))
+                joiner = "." if stage_name else ""
+                meta = Meta(source=f"{stage_name}{joiner}vars[{index}]")
+                context.merge_update(Context(item, meta=meta))
 
     def _resolve_entry(self, name: str, definition):
         context = Context.clone(self.global_ctx)
@@ -115,8 +135,13 @@ class DataResolver:
             )
 
         vars_ = definition.pop(VARS_KWD, [])
+        # FIXME: Should `vars` be templatized?
         self.load_from_vars(
-            context, vars_, wdir, skip_imports=deepcopy(self.imported_files)
+            context,
+            vars_,
+            wdir,
+            skip_imports=deepcopy(self.imported_files),
+            stage_name=name,
         )
 
         logger.trace(  # pytype: disable=attribute-error

dvc/parsing/context.py

@@ -18,6 +18,8 @@ from dvc.parsing.interpolate import (
     is_interpolated_string,
     str_interpolate,
 )
+from dvc.path_info import PathInfo
+from dvc.utils import relpath
 from dvc.utils.serialize import LOADERS
 
 SeqOrMap = Union[Sequence, Mapping]
@@ -50,6 +52,10 @@ class MergeError(ContextError):
         )
 
 
+class ParamsFileNotFound(ContextError):
+    pass
+
+
 class KeyNotInContext(ContextError):
     def __init__(self, key) -> None:
         self.key = key
@@ -79,8 +85,8 @@ class Meta:
         return replace(meta, dpaths=dpaths)
 
     def __str__(self):
-        string = self.source or "<local>:"
-        string += self.path()
+        string = self.source or "<local>"
+        string += ":" + self.path()
         return string
 
     def path(self):
@@ -305,20 +311,19 @@ class Context(CtxDict):
         return node.value if unwrap else node
 
     @classmethod
-    def load_from(cls, tree, file: str) -> "Context":
+    def load_from(cls, tree, path: PathInfo) -> "Context":
+        file = relpath(path)
+        if not tree.exists(path):
+            raise ParamsFileNotFound(f"'{file}' does not exist")
+
         _, ext = os.path.splitext(file)
         loader = LOADERS[ext]
 
-        meta = Meta(source=file, local=False)
-        return cls(loader(file, tree=tree), meta=meta)
+        meta = Meta(source=str(path), local=False)
+        return cls(loader(path, tree=tree), meta=meta)
 
-    def merge_from(self, tree, path, overwrite=False):
-        if not tree.exists(path):
-            raise FileNotFoundError(path)
-
-        self.merge_update(
-            Context.load_from(tree, str(path)), overwrite=overwrite
-        )
+    def merge_from(self, tree, path: PathInfo, overwrite=False):
+        self.merge_update(Context.load_from(tree, path), overwrite=overwrite)
 
     @classmethod
     def clone(cls, ctx: "Context") -> "Context":

tests/unit/test_context.py

@@ -10,6 +10,7 @@ from dvc.parsing.context import (
     CtxList,
     KeyNotInContext,
     MergeError,
+    ParamsFileNotFound,
     Value,
 )
 from dvc.tree.local import LocalTree
@@ -218,8 +219,13 @@ def test_load_from(mocker):
         return {"x": {"y": {"z": 5}, "lst": [1, 2, 3]}, "foo": "foo"}
 
     mocker.patch("dvc.parsing.context.LOADERS", {".yaml": _yaml_load})
+
+    class tree:
+        def exists(self, _):
+            return True
+
     file = "params.yaml"
-    c = Context.load_from(object(), file)
+    c = Context.load_from(tree(), file)
 
     assert asdict(c["x"].meta) == {
         "source": file,
@@ -406,5 +412,5 @@ def test_resolve_resolves_dict_keys():
 
 def test_merge_from_raises_if_file_not_exist(tmp_dir, dvc):
     context = Context(foo="bar")
-    with pytest.raises(FileNotFoundError):
-        context.merge_from(dvc.tree, DEFAULT_PARAMS_FILE)
+    with pytest.raises(ParamsFileNotFound):
+        context.merge_from(dvc.tree, tmp_dir / DEFAULT_PARAMS_FILE)