summon: first draft with python specific implementation · Pull Request #2971 · treeverse/dvc

ghost · 2019-12-18T06:47:06Z

TODO:

Things omitted on this PR:

Support for different types other than python.
Checking requirements.

shcheklein · 2019-12-18T19:39:31Z

@MrOutis enable the pre-commit to avoid restyled triggered?

ghost · 2019-12-18T19:40:39Z

@shcheklein , yes, already did :)

dmpetrov

👍

dmpetrov · 2019-12-18T21:51:22Z

+def summon(name, repo=None, rev=None):
+    # 1. Read grimoire.yaml
+    # 2. Pull dependencies (TODO)
+    # 3. Get the call and parameters


How do we suppose to "deserialize" not-python objects when the yml file contains type: CSV for examlpe?

It is important from an external library developer's point of view - when I develop a library to deserialize CSV or Parquet for example.

@dmpetrov , decided not to implement type: csv in this first iteration because it was ambiguous.

Sounds good. But please make sure it won't affect design decisions - see my comment about yml file format and file/method/params.

@dmpetrov dvc.api.summon() is not supposed to deserialize type: python, we agreed to move this functionality into a separate library.

@Suor right. But what the library supposes to call from DVC? I expected to have some generalized approach of getting objects. So, we can create the library and custom types easily on top of this DVC call.

@Suor , did you mean type: csv ?

pared

I've got a question about default name,
Is actually name, what we want to use by default?
Shouldn't it be {something}.dvcsummon? In case of no fname provided we could be gathering all the dvcsummon's and try to find proper object among them by name

ghost · 2019-12-19T15:03:40Z

I've got a question about default name,
Is actually name, what we want to use by default?
Shouldn't it be {something}.dvcsummon? In case of no fname provided we could be gathering all the dvcsummon's and try to find proper object among them by name

@pared, that's interesting. Not sure what we are planning to support.
As far as I understood, you can only use one summon.yaml at the time, so there's no need to search through all summon files as we do with dvcfiles .

@dmpetrov , @shcheklein , are we supporting this case? ☝️

Suor · 2019-12-19T15:48:39Z

+    old_dir = os.path.abspath(os.curdir)
+    os.chdir(path)
+    sys.path.insert(0, path)


You don't need to do both probably. Also, use try/finally

It is needed, @Suor, or at least I couldn't rely solely on os.chdir.
Looks like importlib.import_module doesn't care of the current directory (well, sys.path is not being updated when running os.chdir)

By the way, do you mean try/finally instead of the context manager, or inside the context manager?

dmpetrov

As far as I understood, you can only use one summon.yaml at the time, so there's no need to search through all summon files as we do with dvcfiles .

We discussed this above - I suggested to change the method signature.

Yes, we'd like to avoid the summon file search. At the same time, we cannot use a predefined file in a repo root dir - it blocks the entire scenario for mono-repo teams. So, summon.yaml is default file that can be changed by the user when user calls the methiod.

dmpetrov · 2019-12-18T22:51:37Z

+def summon(name, repo=None, rev=None):
+    # 1. Read grimoire.yaml
+    # 2. Pull dependencies (TODO)
+    # 3. Get the call and parameters


Sounds good. But please make sure it won't affect design decisions - see my comment about yml file format and file/method/params.

dmpetrov · 2019-12-19T03:11:36Z

+def summon(name, repo=None, rev=None):
+    # 1. Read grimoire.yaml
+    # 2. Pull dependencies (TODO)
+    # 3. Get the call and parameters


@Suor right. But what the library supposes to call from DVC? I expected to have some generalized approach of getting objects. So, we can create the library and custom types easily on top of this DVC call.

dmpetrov · 2019-12-24T00:47:49Z

@MrOutis I want to clarify a piece of requirements that might be not clear from the description. We need to support custome types such as CSV. I want to emphasize, that we should have all the functions in dvc.api to implement a custom type. So, dvc.api.summon should be general enough (we can have a separate general dvc.api.summon_ex and python specific dvc.api.summon) OR we should have a set of methods.

Suor

Make a SummonError exception class, use it for all summon errors with appropriate messages. Add a test or a couple, which will catch those.

Suor · 2019-12-24T08:22:35Z

+    # TODO: Write a meaningful docstring about `summon`
+    with _make_repo(repo, rev=rev) as _repo:
+
+        def pull_dependencies(deps):


Might be moved outside.

This is actually repo.pull(deps) after granularity patch is in, see dvc: support granularity for fetch/pull/push/status/checkout #3002

Suor · 2019-12-24T08:32:12Z

+        try:
+            objects = ruamel.yaml.safe_load(fobj.read())["objects"]
+            objects = schema(objects)
+            return next(x for x in objects if x["name"] == name)


Again, need to check there is only one object named name.

Do we need to raise an error? Isn't it just better to pick the first one?

Yes we need to raise an error, otherwise it might be a nasty surprise for a user.

Suor · 2019-12-24T08:38:25Z

+
+    try:
+        os.chdir(path)
+        sys.path.insert(0, path)


Again, don't need sys.path.insert() if we already chdired into in.

@Suor , left a commend right here: #2971 (comment)

Only it works as long as you have '.' or '' in sys.path, which is the default. Without that lots of python code won't work. Are you sure imports don't work for you without that? Can you test outside dvc on a simple script?

Also might be the reason why random tests failed.

Suor · 2019-12-24T08:42:18Z

            yield repo
+
+
+def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None):


Suggested change

def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None):

def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):

Suor · 2019-12-24T08:43:49Z

+    # XXX: Some issues with this approach:
+    #   * Not thread safe
+    #   * Import will pollute sys.modules
+    #   * Weird errors if there is a name clash within sys.modules


There is no good non complicated way to handle this in Python 2. I will come up with an approach next sprint. Leave as is for now.

* Mark optional keywords * Allow extra entries * Include objects into the schema

Suor · 2019-12-25T03:01:55Z

+        ]
+    },
+    required=True,
+    extra=ALLOW_EXTRA,


We should not allow extra everywhere. May not allow it anywhere for now for better future-compatibility. Say only allow objects at the top, and only allow name, meta and summon in the object, all the user data should go under meta, like description, paper, author, etc.

Suor · 2019-12-25T03:06:27Z

+        pull_dependencies(info.get("deps"))
+
+        _args = copy.deepcopy(info.get("args", {}))
+        _args.update(args or {})


Either remove or {} here or a default value to .get() on prev line.

Suor · 2019-12-25T03:11:02Z

+        # XXX: Prints ugly absolute path.
+        raise SummonError("No such YAML file with path: '{}'".format(path))


Should contain whatever user passed, e.g. file ... not found in repo ...

Suor · 2019-12-25T03:11:34Z

+        # XXX: Prints ugly absolute path.
+        raise SummonError("No such YAML file with path: '{}'".format(path))
+    except ruamel.yaml.YAMLError as exc:
+        raise SummonError("Failed to parse YAML correctly", exc)


Need to say which file in which repo and a line/message of an error if available.

Suor · 2019-12-25T03:13:06Z

+        raise SummonError("Failed to parse YAML correctly", exc)
+    except Invalid as exc:
+        # XXX: Doesn't point out the error.
+        raise SummonError("YAML file dosen't match with the schema", exc)


This is not just yaml file, this is a "summon file" is broken.

Suor · 2019-12-25T03:14:20Z

+        # 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))


Again, need to add summon_file and repo. We might actually add it outside where this info is available. This will fix all of the above and wouldn't need those two be passed here.

Suor · 2019-12-25T03:23:35Z

+
+    try:
+        os.chdir(path)
+        sys.path.insert(0, path)


Only it works as long as you have '.' or '' in sys.path, which is the default. Without that lots of python code won't work. Are you sure imports don't work for you without that? Can you test outside dvc on a simple script?

Suor · 2019-12-25T08:05:03Z

It looks like something is wrong with Python 2.7. Besides that looks good)

dmpetrov · 2019-12-25T09:43:26Z

@MrOutis it looks like you had a great time on Christmas eve (I'm not sure which emoji to put here)
🎄👨‍💻?

Suor · 2019-12-26T10:05:44Z

Are all the remaining fails unrelated?

ghost · 2019-12-27T15:52:19Z

Are all the remaining fails unrelated?

No, @Suor , looks like I've introduced some bugs 🤔
Tests are running correctly on master but my branch fails to run the test suite without errors

ghost · 2019-12-27T16:05:50Z

No, @Suor , looks like I've introduced some bugs thinking
Tests are running correctly on master but my branch fails to run the test suite without errors

Looks like sys.path.pop wasn't on the correct index

restyled-io Bot mentioned this pull request Dec 18, 2019

Restyle [wip] summon: ugly hack #2972

Closed

Mr. Outis added 2 commits December 18, 2019 00:48

summon: ugly hack

3d13318

summon: safely get a module; change naming

b921730

dmpetrov suggested changes Dec 18, 2019

View reviewed changes

ghost changed the title ~~[wip] summon: ugly hack~~ [wip] summon: first draft with python specific implementation Dec 18, 2019

Mr. Outis added 3 commits December 18, 2019 16:41

summon: support checking out dependencies

789db91

summon: grimoire/spells -> artifacts

d0344ce

summon: support overriding parameters

5d66a57

ghost requested review from Suor, efiop and pared December 18, 2019 23:27

Suor suggested changes Dec 19, 2019

View reviewed changes

Mr. Outis added 10 commits December 18, 2019 22:15

summon: return empty generator for object.deps

fea3d14

summon: use find_out_by_relpath

4ce1113

summon: remove scm.add, *_gen already handles it

32f35ea

💅 black

0a54683

summon: artifacs.yaml -> dvcsummon.yaml; artifact -> object

1b3c771

summon: file & method -> call

29339d2

summon: params -> args; use summon key in yaml

4273ff4

summon: left a comment about chdir and syspath

ee72280

💅 black

c852d33

summon: allow different YAML file names

addc56c

pared reviewed Dec 19, 2019

View reviewed changes

Suor suggested changes Dec 19, 2019

View reviewed changes

dmpetrov suggested changes Dec 19, 2019

View reviewed changes

weekly-digest Bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #2996

Closed

Mr. Outis added 2 commits December 24, 2019 00:42

summon: wrap chdir_and_syspath in a try/finally

542f4c3

summon: 💅 refactoring/reordering

7dd340a

ghost requested a review from Suor December 24, 2019 08:18

summon: adjust comments

1c22cd5

Suor suggested changes Dec 24, 2019

View reviewed changes

Mr. Outis added 4 commits December 24, 2019 13:41

summon: make schema more consistent

d45f07c

* Mark optional keywords * Allow extra entries * Include objects into the schema

tests/summon: use different args for non-canon.yml

fbb7aeb

summon: raise SummonError appropiately

1e65c76

💅 black & flake

7a58901

Suor suggested changes Dec 25, 2019

View reviewed changes

Mr. Outis added 3 commits December 25, 2019 00:42

summon: pair-programming adjustments

0658622

💅 black

75ad14a

💅 refactor

4a25612

Mr. Outis added 2 commits December 25, 2019 13:07

py2: fix problem with ruamel.yaml and unicode

49264b5

Merge branch 'master' into summon

b977471

efiop reviewed Dec 27, 2019

View reviewed changes

Comment thread dvc/api.py Outdated

efiop reviewed Dec 27, 2019

View reviewed changes

Comment thread dvc/api.py

summon: fix path popping operation

e8dd235

ghost changed the title ~~[wip] summon: first draft with python specific implementation~~ summon: first draft with python specific implementation Dec 27, 2019

summon: os.curdir -> os.getcwd

dad224e

efiop approved these changes Dec 27, 2019

View reviewed changes

pared approved these changes Dec 27, 2019

View reviewed changes

efiop merged commit 1038cfc into treeverse:master Dec 27, 2019

ghost deleted the summon branch December 27, 2019 18:59

weekly-digest Bot mentioned this pull request Dec 29, 2019

Weekly Digest (22 December, 2019 - 29 December, 2019) #3013

Closed

		yield repo


		def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None):

	def summon(name, fname="dvcsummon.yaml", args=None, repo=None, rev=None):
	def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):

		# XXX: Prints ugly absolute path.
		raise SummonError("No such YAML file with path: '{}'".format(path))

Conversation

ghost commented Dec 18, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shcheklein commented Dec 18, 2019

Uh oh!

ghost commented Dec 18, 2019

Uh oh!

dmpetrov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmpetrov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmpetrov commented Dec 24, 2019

Uh oh!

Suor left a comment

Choose a reason for hiding this comment

Uh oh!

Suor Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Dec 24, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 18, 2019 •

edited by ghost

Loading

Suor Dec 24, 2019 •

edited

Loading

ghost Dec 24, 2019 •

edited by ghost

Loading

Suor commented Dec 25, 2019 •

edited

Loading

dmpetrov commented Dec 25, 2019 •

edited

Loading