diff --git a/dvc/api.py b/dvc/api.py index a9bb95c75..17b1231d4 100644 --- a/dvc/api.py +++ b/dvc/api.py @@ -10,7 +10,7 @@ except ImportError: from contextlib import GeneratorContextManager as GCM import ruamel.yaml -from voluptuous import Schema, Optional, Invalid, ALLOW_EXTRA +from voluptuous import Schema, Required, Invalid from dvc.utils.compat import urlparse, builtin_str from dvc.repo import Repo @@ -18,22 +18,21 @@ from dvc.exceptions import SummonError, FileMissingError from dvc.external_repo import external_repo -summon_schema = Schema( +SUMMON_SCHEMA = Schema( { - "objects": [ + Required("objects"): [ { - "name": str, - "summon": { - "type": "python", - "call": str, - Optional("args"): dict, - Optional("deps"): [str], + Required("name"): str, + "meta": dict, + Required("summon"): { + Required("type"): "python", + Required("call"): str, + "args": dict, + "deps": [str], }, } ] }, - required=True, - extra=ALLOW_EXTRA, ) @@ -97,26 +96,20 @@ def _make_repo(repo_url, rev=None): yield repo -def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None): - # TODO: Write a meaningful docstring about `summon` +def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None): + """Instantiate an object described in the summon file.""" with _make_repo(repo, rev=rev) as _repo: + try: + path = os.path.join(_repo.root_dir, summon_file) + obj = _get_object_from_summon_file(name, path) + info = obj["summon"] + except SummonError as exc: + raise SummonError( + str(exc) + " at '{}' in '{}'".format(summon_file, repo), + cause=exc.cause + ) - def pull_dependencies(deps): - if not deps: - return - - outs = [_repo.find_out_by_relpath(dep) for dep in deps] - - with _repo.state: - for out in outs: - _repo.cloud.pull(out.get_used_cache()) - out.checkout() - - path = os.path.join(_repo.root_dir, fname) - obj = _get_object_from_summoners_file(name, path) - info = obj["summon"] - - pull_dependencies(info.get("deps")) + _pull_dependencies(_repo, info.get("deps")) _args = copy.deepcopy(info.get("args", {})) _args.update(args or {}) @@ -124,26 +117,43 @@ def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None): return _invoke_method(info["call"], _args, path=_repo.root_dir) -def _get_object_from_summoners_file(name, path): +def _get_object_from_summon_file(name, path): """ Given a summonable object's name, search for it on the given file - and bring it to life. + and return its description. """ try: with open(path, "r") as fobj: - content = summon_schema(ruamel.yaml.safe_load(fobj.read())) - objects = content["objects"] - return next(x for x in objects if x["name"] == name) + content = SUMMON_SCHEMA(ruamel.yaml.safe_load(fobj.read())) + objects = [x for x in content["objects"] if x["name"] == name] + + if not objects: + raise SummonError("No object with name '{}'".format(name)) + elif len(objects) >= 2: + raise SummonError( + "More than one object with name '{}'".format(name) + ) + + return objects[0] + except FileMissingError: - # XXX: Prints ugly absolute path. - raise SummonError("No such YAML file with path: '{}'".format(path)) + raise SummonError("Summon file not found") except ruamel.yaml.YAMLError as exc: - raise SummonError("Failed to parse YAML correctly", exc) + raise SummonError("Failed to parse summon file", exc) except Invalid as exc: - # XXX: Doesn't point out the error. - raise SummonError("YAML file dosen't match with the schema", exc) - except StopIteration: - raise SummonError("No such object with name '{}'".format(name)) + raise SummonError(str(exc)) + + +def _pull_dependencies(repo, deps): + if not deps: + return + + outs = [repo.find_out_by_relpath(dep) for dep in deps] + + with repo.state: + for out in outs: + repo.cloud.pull(out.get_used_cache()) + out.checkout() def _invoke_method(call, args, path): @@ -151,6 +161,9 @@ def _invoke_method(call, args, path): # * Not thread safe # * Import will pollute sys.modules # * Weird errors if there is a name clash within sys.modules + + # XXX: sys.path manipulation is "theoretically" not needed + # but tests are failing for an unknown reason. cwd = os.path.abspath(os.curdir) try: @@ -163,23 +176,17 @@ def _invoke_method(call, args, path): sys.path.pop() -def _import_string(import_name, silent=False): +def _import_string(import_name): """Imports an object based on a string. Useful to delay import to not load everything on startup. Use dotted notaion in `import_name`, e.g. 'dvc.remote.gs.RemoteGS'. - If the `silent` is True the return value will be `None` if the import - fails. :return: imported object """ import_name = builtin_str(import_name) - try: - if "." in import_name: - module, obj = import_name.rsplit(".", 1) - else: - return importlib.import_module(import_name) - return getattr(importlib.import_module(module), obj) - except (ImportError, AttributeError): - if not silent: - raise + if "." in import_name: + module, obj = import_name.rsplit(".", 1) + else: + return importlib.import_module(import_name) + return getattr(importlib.import_module(module), obj) diff --git a/tests/func/test_api.py b/tests/func/test_api.py index 0d90b26ad..1d22be44a 100644 --- a/tests/func/test_api.py +++ b/tests/func/test_api.py @@ -137,7 +137,7 @@ def test_summon(tmp_dir, erepo_dir, dvc, monkeypatch): "objects": [ { "name": "sum", - "description": "Add to ", + "meta": { "description": "Add to " }, "summon": { "type": "python", "call": "calculator.add_to_num", @@ -151,8 +151,12 @@ def test_summon(tmp_dir, erepo_dir, dvc, monkeypatch): different_objects = copy.deepcopy(objects) different_objects["objects"][0]["summon"]["args"]["x"] = 100 + dup_objects= copy.deepcopy(objects) + dup_objects["objects"] *= 2 + dvcsummon_yaml = ruamel.yaml.dump(objects) different_yaml = ruamel.yaml.dump(different_objects) + dup_yaml = ruamel.yaml.dump(dup_objects) invalid_yaml = ruamel.yaml.dump({"name": "sum"}) not_yaml = "a: - this is not a valid YAML file" @@ -162,6 +166,7 @@ def test_summon(tmp_dir, erepo_dir, dvc, monkeypatch): erepo_dir.dvc_gen("number", "100", commit="Add number.dvc") erepo_dir.scm_gen("dvcsummon.yaml", dvcsummon_yaml) erepo_dir.scm_gen("different.yaml", different_yaml) + erepo_dir.scm_gen("dup.yaml", dup_yaml) erepo_dir.scm_gen("invalid.yaml", invalid_yaml) erepo_dir.scm_gen("not_yaml.yaml", not_yaml) erepo_dir.scm_gen( @@ -174,16 +179,25 @@ def test_summon(tmp_dir, erepo_dir, dvc, monkeypatch): assert api.summon("sum", repo=repo_url) == 101 assert api.summon("sum", repo=repo_url, args={"x": 2}) == 102 - assert api.summon("sum", repo=repo_url, fname="different.yaml") == 200 + assert api.summon("sum", repo=repo_url, summon_file="different.yaml") == 200 - with pytest.raises(SummonError): - api.summon("sum", repo=repo_url, fname="not-existent-file.yaml") + try: + api.summon("sum", repo=repo_url, summon_file="missing.yaml") + except SummonError as exc: + assert "Summon file not found" in str(exc) + assert "missing.yaml" in str(exc) + assert repo_url in str(exc) + else: + pytest.fail("Did not raise on missing summon file") - with pytest.raises(SummonError): - api.summon("non-existent", repo=repo_url) + with pytest.raises(SummonError, match=r"No object with name 'missing'"): + api.summon("missing", repo=repo_url) - with pytest.raises(SummonError): - api.summon("sum", repo=repo_url, fname="invalid.yaml") + with pytest.raises(SummonError, match=r"More than one object with name 'sum'"): + api.summon("sum", repo=repo_url, summon_file="dup.yaml") - with pytest.raises(SummonError): - api.summon("sum", repo=repo_url, fname="not_yaml.yaml") + with pytest.raises(SummonError, match=r"extra keys not allowed"): + api.summon("sum", repo=repo_url, summon_file="invalid.yaml") + + with pytest.raises(SummonError, match=r"Failed to parse summon file"): + api.summon("sum", repo=repo_url, summon_file="not_yaml.yaml")