Resolver pattern #404

wild-endeavor · 2021-02-26T21:02:47Z

TL;DR

This changes the execution side of flytekit. Previously tasks were loaded only with a module and key to look up through an importlib call. This introduces a "task resolver" construct which is responsible for two things:

Given a task, when serializing be able to produce a series of strings that, at execution time,
allow the resolver to reconstruct the task object.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Please see the comments on the TaskResolverMixin.
When a task is serialized, the command that is generated is now the output of the task's resolver.
The entrypoint.py click command has been updated to be able to handle that command.

As part of this change, it was found that it was useful to be able to keep track of the variable that a task resolver was assigned to. Because of this, we've added back (duplicated rather) the instance-tracking metaclass mechanism from the old API (the stuff in registerable.py). See the new tests under the tracking/ folder for a non-Flyte-based example of how it works.

To simplify things for legacy API tasks, the entrypoint.py execute function has been separated out.
PythonAutoContainerTask has been moved into a separate file.
Because PythonAutoContainerTasks are now a TrackedInstance we no longer need the InstanceVar construct.

Tracking Issue

https://github.com/lyft/flyte/issues/

Follow-up issue

NA

Signed-off-by: wild-endeavor <[email protected]>

"

Signed-off-by: wild-endeavor <[email protected]>

flytekit/bin/entrypoint.py

katrogan · 2021-03-05T21:02:25Z

flytekit/clis/sdk_in_container/serialize.py

@@ -158,11 +156,6 @@ def serialize_all(
            )
            old_style_entities.append(o)

-        # PythonInstanceTasks will not be picked up by the above, so we need to reiterate
-        for o, v in load_module_object_for_type(pkgs, PythonInstanceTask, additional_path=local_source_root).items():


just to double check, we don't need this for #391?

nope, getting rid of it, using the instance tracking metaclass now.

flytekit/core/python_auto_container.py

Co-authored-by: Katrina Rogan <[email protected]>

Signed-off-by: wild-endeavor <[email protected]>

wild-endeavor · 2021-03-10T22:42:38Z

flyteorg/flyte#814 end to end tests.

kumare3 · 2021-03-11T00:52:27Z

flytekit/bin/entrypoint.py

@@ -5,6 +5,7 @@
 import pathlib
 import random as _random
 import traceback as _traceback
+from typing import List


so hypothetically if we were to use the TaskResolver to execute fast-registration, then in that case, we probably need some way of cascading the resolver down the dynamic chain right?
I understand by definition at the moment TaskResolver is bound to a Task. but in the case of dynamic task, would it automatically cascade downstream?

I need to think about this more. Can I do this as a separate PR? I'll write up an issue... also because fast-register doesn't work right now with dynamic tasks.

kumare3 · 2021-03-11T00:53:54Z

flytekit/core/map_task.py

            "--inputs",
            "{{.input}}",
            "--output-prefix",
            "{{.outputPrefix}}",
            "--raw-output-data-prefix",
            "{{.rawOutputDataPrefix}}",
+            "--resolver",


this implies that the resolver has to be the last arg only?

why not use multi-valued args?
--resolver-arg "x=y" --resolver-arg "..."

not sure what you mean. the resolver can be specified anywhere, but the resolver args have to be at the end.

I think @kumare3 means an alternative syntax is
--resolver ... --resolver-arg "task-module=<TASK_MODULE>" -- resolver-arg "task-name=<TASK_NAME>"

Where --resolver-arg is a click option instead of an argument.

Will the resolver always have 2 args, task-module and task-name?

just throwing out another idea:

--resolver ... --resolver-task-module ... --resolver task-name ...

kumare3 · 2021-03-11T01:01:19Z

flytekit/core/python_function_task.py

            "--inputs",
            "{{.input}}",
            "--output-prefix",
            "{{.outputPrefix}}",
            "--raw-output-data-prefix",
            "{{.rawOutputDataPrefix}}",
+            "--resolver",
+            self.task_resolver.location,
+            "--",


i think i really prefer --resolver-arg type of usage, why do you want this raw command line?

Spent half an hour getting it to work with click and gave up?

tests/flytekit/unit/core/tracker/a.py

kumare3 · 2021-03-11T01:04:39Z

This looks great to me, couple nits

flytekit/core/python_auto_container.py

cosmicBboy · 2021-03-10T23:10:53Z

flytekit/core/python_auto_container.py

+
+        task_module = importlib.import_module(task_module)
+        task_def = getattr(task_module, task_name)
+        return task_def


task_def and task_module vars only used in once, consider return getattr(importlib.import_module(task_module), task_name)

cosmicBboy · 2021-03-10T23:12:51Z

flytekit/core/python_function_task.py

-    """
-    return func.__code__.co_flags & inspect.CO_NESTED != 0
-
+        container_args = [


can do

return [ "pyflyte-execute", ... "--", *self.task_resolver.loader_args(settings, self), ]

cosmicBboy · 2021-03-10T23:13:50Z

flytekit/core/python_function_task.py

@@ -244,20 +144,23 @@ def execute(self, **kwargs) -> Any:
            return self.dynamic_execute(self._task_function, **kwargs)

    def get_command(self, settings: SerializationSettings) -> List[str]:
-        return [
+        container_args = [


can do

return [ "pyflyte-execute", ... "--", *self.task_resolver.loader_args(settings, self), ]

flytekit/core/map_task.py

cosmicBboy · 2021-03-11T15:24:35Z

flytekit/core/python_auto_container.py

+        )
+
+
+class TaskResolverMixin(object):


python 3+: object is default parent class

Suggested change

class TaskResolverMixin(object):

class TaskResolverMixin

i like keeping it though, makes it explicit and it's everywhere else.

flytekit/core/context_manager.py

cosmicBboy · 2021-03-11T15:50:26Z

flytekit/core/map_task.py

            "--inputs",
            "{{.input}}",
            "--output-prefix",
            "{{.outputPrefix}}",
            "--raw-output-data-prefix",
            "{{.rawOutputDataPrefix}}",
+            "--resolver",


I think @kumare3 means an alternative syntax is
--resolver ... --resolver-arg "task-module=<TASK_MODULE>" -- resolver-arg "task-name=<TASK_NAME>"

Where --resolver-arg is a click option instead of an argument.

Will the resolver always have 2 args, task-module and task-name?

Signed-off-by: wild-endeavor <[email protected]>

Co-authored-by: Niels Bantilan <[email protected]>

wild-endeavor · 2021-03-11T20:27:29Z

Just to answer the above - the resolver arg is not an option yeah, it's a click arg. The number of arguments is unknown except that we require it to be >=1. it could be just one or someone may make a new resolver a year from now that has a hundred args.

v01dXYZ · 2021-03-14T19:45:15Z

Would it be useful to add a warning about cloudpickle ? It's not a robust way for serializing Python classes/functions compared to textual/bytecode code (or a package). Indeed, only the objects in __main__ would be serialized, which means if an user wants to gather helper functions in a utils.py that won't work anymore + the warnings from cloudpickle docs (https://github.com/cloudpipe/cloudpickle)

cloudpickle is excellent if we want to send functions from a REPL to a cluster but not with dealing with a modular source tree.

kumare3 · 2021-10-10T04:22:47Z

Would it be useful to add a warning about cloudpickle ? It's not a robust way for serializing Python classes/functions compared to textual/bytecode code (or a package). Indeed, only the objects in __main__ would be serialized, which means if an user wants to gather helper functions in a utils.py that won't work anymore + the warnings from cloudpickle docs (https://github.com/cloudpipe/cloudpickle)

cloudpickle is excellent if we want to send functions from a REPL to a cluster but not with dealing with a modular source tree.
@v01dXYZ we did not see this, I do agree. that is why Flytekit does not really recommend or support cloudpickle in any meaningful way. but we would love to talk more, would be open to discuss in slack?

kumare3 and others added 30 commits February 4, 2021 15:51

How we could load arbitrary tasks

11495ce

Updated to work with TaskLoader

1ae2f43

This should work

00c1aeb

fmt

cc470af

loader example

cd21fd3

Better naming and location

6fafd82

Making workflow closures possible

6ecf051

not compiling need to work on something else

faef09c

eod

f423958

wip

27db994

test

f8d8949

test

455c34c

Signed-off-by: wild-endeavor <[email protected]>

test

e5e7147

Signed-off-by: wild-endeavor <[email protected]>

cleaned up entrypoint

920aeb6

fix raise test"

17783d0

"

fmt

45ac185

wip

d6de919

move files around

69ca563

Signed-off-by: wild-endeavor <[email protected]>

move tracked instance for resolvers

b41aac0

Signed-off-by: wild-endeavor <[email protected]>

remove d, rename map

1253d4a

Signed-off-by: wild-endeavor <[email protected]>

merge master

b126795

Signed-off-by: wild-endeavor <[email protected]>

remove duplicate execute_task_cmd

e95d172

Signed-off-by: wild-endeavor <[email protected]>

wip

044a227

Signed-off-by: wild-endeavor <[email protected]>

wip

b49a783

Signed-off-by: wild-endeavor <[email protected]>

dashes

ae3e251

Signed-off-by: wild-endeavor <[email protected]>

wip

1bbecb0

Signed-off-by: wild-endeavor <[email protected]>

wip

2541bc7

Signed-off-by: wild-endeavor <[email protected]>

respecting compilation state task resolver

57f33dc

tests

18c1527

Signed-off-by: wild-endeavor <[email protected]>

tests

463f5ce

Signed-off-by: wild-endeavor <[email protected]>

katrogan reviewed Mar 5, 2021

View reviewed changes

nit

4a54d26

cosmicBboy reviewed Mar 8, 2021

View reviewed changes

flytekit/core/python_auto_container.py Outdated Show resolved Hide resolved

cosmicBboy reviewed Mar 8, 2021

View reviewed changes

flytekit/core/python_auto_container.py Show resolved Hide resolved

wild-endeavor and others added 8 commits March 10, 2021 11:11

Apply suggestions from code review

c1e7827

Co-authored-by: Katrina Rogan <[email protected]>

pr comments and expose at extend

eb09c27

more pr comments

ca8f562

fix pod test

fe28580

merge master

706d1ea

adding useless tests

c0da4e9

stop complaining about test files not being tested, also cli

a34ae78

test

5470eb4

Signed-off-by: wild-endeavor <[email protected]>

kumare3 reviewed Mar 11, 2021

View reviewed changes

tests/flytekit/unit/core/tracker/a.py Show resolved Hide resolved

cosmicBboy reviewed Mar 11, 2021

View reviewed changes

wild-endeavor and others added 5 commits March 11, 2021 10:17

pr comments

d8cb25b

Signed-off-by: wild-endeavor <[email protected]>

list interpret the container args

6d409f8

Signed-off-by: wild-endeavor <[email protected]>

fmt

82c825d

Signed-off-by: wild-endeavor <[email protected]>

revert

db8306f

Update flytekit/core/python_auto_container.py

38976fd

Co-authored-by: Niels Bantilan <[email protected]>

cosmicBboy approved these changes Mar 11, 2021

View reviewed changes

wild-endeavor merged commit ce42886 into master Mar 11, 2021

max-hoffman pushed a commit to dolthub/flytekit that referenced this pull request May 11, 2021

Resolver pattern (flyteorg#404)

bcaf71f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolver pattern #404

Resolver pattern #404

wild-endeavor commented Feb 26, 2021 •

edited

Loading

katrogan Mar 5, 2021

wild-endeavor Mar 10, 2021

wild-endeavor commented Mar 10, 2021

kumare3 Mar 11, 2021

wild-endeavor Mar 11, 2021

kumare3 Mar 11, 2021

kumare3 Mar 11, 2021

wild-endeavor Mar 11, 2021

cosmicBboy Mar 11, 2021

cosmicBboy Mar 11, 2021

kumare3 Mar 11, 2021

wild-endeavor Mar 11, 2021

kumare3 commented Mar 11, 2021

cosmicBboy Mar 10, 2021

cosmicBboy Mar 10, 2021

cosmicBboy Mar 10, 2021

cosmicBboy Mar 11, 2021

wild-endeavor Mar 11, 2021

cosmicBboy Mar 11, 2021

wild-endeavor commented Mar 11, 2021

v01dXYZ commented Mar 14, 2021 •

edited

Loading

kumare3 commented Oct 10, 2021

Resolver pattern #404

Resolver pattern #404

Conversation

wild-endeavor commented Feb 26, 2021 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor commented Mar 11, 2021

v01dXYZ commented Mar 14, 2021 • edited Loading

kumare3 commented Oct 10, 2021

wild-endeavor commented Feb 26, 2021 •

edited

Loading

v01dXYZ commented Mar 14, 2021 •

edited

Loading