`add_to_package` decorator hook. #834

valayDave · 2021-11-23T08:03:12Z

Cherry-picking diff from Added decorator hook for adding custom package #649 to reopen PR.

- Cherry picking diff from card branch.

valayDave · 2021-11-23T08:03:33Z

cc @pjoshi30

valayDave · 2021-11-23T08:05:36Z

@romain-intel / @savingoyal : please review when you get the time.

romain-intel · 2021-11-23T09:25:49Z

metaflow/package.py

+                    file_path, _ = path_tuple
+                    # Check if the path is not duplicated as
+                    # many steps can have the same packages being imported
+                    if file_path not in deco_module_paths:


I was commenting on this in the cards PR (not yet submitted) but what happens if multiple decorators want to include the same file but as different name? I am not sure whey this can happen but it seems a possibility if you expose the name (which is then ignored) to the plugins.

Interesting point.

Some context to why I have kept the path_tuples as a return type from add_to_package:

path_tuples consists of path,arcname

arcname is useful when we untar the package on a remote execution as the relative path in the arcname makes a difference in how imports will happen on the remote container. Here is an example in the Cards PR where I needed to ensure correct path in the arcname to ensure remote imports works correctly.

Now coming to your point. Yes, I don't currently check for same file but as different name and I can change that. I can check for both the file_path as well as the arcname by adding an object like arcname-filepath in deco_module_paths.

Does that resolve this issue ?

romain-intel · 2021-11-23T09:26:13Z

metaflow/decorators.py

@@ -239,6 +239,12 @@ def package_init(self, flow, step_name, environment):
        """
        pass

+    def add_to_package(self):
+        """
+        Called to add custom packages needed for a decorator


Be more descriptive about what this should return. It seems its a tuple of (path, name) although see my comment about the name.

Quick follow up, what problem does this PR help to solve? Is it an alternative to system pip install?

This is not an alternative to pip install.

Context

This PR introduces a decorator hook that allows the decorator to include files in the metaflow code package. As decorators plug into different abstractions of the Metaflow stack, we may require custom package-related files based on the type of decorator we are implementing. This is where that hook becomes essential.

Example

In #815 the @card decorator introduces the concept of custom card modules (metaflow_cards). These modules are external packages that may be present on your local python path; Including those for remote execution requires those modules to be a part of the Metaflow code package ( or installed externally via @conda)

Using this decorator hook, We detect and bundle along those modules with the metaflow code package.

This is not an alternative to pip install.
In #815 the @card decorator introduces the concept of custom card modules (metaflow_cards). These modules are external packages that may be present on your local python path; Including those for remote execution requires those modules to be a part of the Metaflow code package ( or installed externally via @conda)

how would one specify a custom package here that is not installable via conda?

I am a little confused to how to answer this so I am decompressing what I wrote earlier. Apologies if this seems like an information dump or if I don't cover what you may be seeking for.

Just for clarification: As this is a decorator hook, these custom packages may be present to fulfill the requirements of the decorator. So they may not be a part of "userland" i.e the users don't need to explicitly specify the packages in the decorator invocation but rather the decorator automatically seeks them and includes them with metaflow code package.

These packages may have been pip installed into the local python environment or present in PYTHONPATH or lie somewhere in the filesystem. Continuing the example of @card decorator, @card decorator seeks for metaflow_cards modules present in the local python path / pip packages. The metaflow_cards module in the current context is a plugin-like module which behaves like a namespace package. As a part of the MF run lifecycle we seek these modules from the current python path / pip packages and include them in the code package. We can even install them via conda if they are published on a conda channel.

The main purpose of this hook is to include files to package before run starts ( code files that are collected through the MetaflowPackage class ).

Why do we use this hook

The @card decorator uses other types of files except for python files such as html , css, and js files to create a HTML that visualize the results of a Task; This requires that we seek filetypes that are generally not included in the metaflow code package. Hence this hook helps seek and include them as a part of the code package. Even the custom metaflow_cards modules that we install can have other types of files like html , css, js, etc which get included by default because of this decorator hook.

Example of metaflow_cards Namespace package.

As metaflow_cards is a namespace package; we can install it's subpackages using pip install metaflow-cards-container-monitoring; Once these packages are installed in the local python path or in pip library, they will be seeked by the card decorator and then included in the metaflow code package.

Gotacha's with directly seeking Installed Modules

As we are directly seeking modules and including them in the code package, the dependencies needed by these packages may not be covered ; This is something which we don't seek (at the moment)( haven't thought deeply upon this) .

Hope this addresses what you were seeking for ? :)

yes, thank you for the details! My initial understanding of this change was incorrect - I had thought this would allow anyone to specify a list of packages required by the decorator and would attempt to pip install these list of packages. After talking to @romain-intel I have more context on this change. IIUC, this change includes the files required by the decorator in the code package. The files themselves could have been installed through one of several ways: pip, conda or simply copying the files on the FS.

While this doesn't solve the larger custom package and dependency management problem that we have, I think this work well for a certain set of use cases that we have internally.

Thanks for the PR and the explanation!

- Fixed comment.

pjoshi30 · 2021-11-24T20:12:58Z

metaflow/decorators.py

@@ -239,6 +239,13 @@ def package_init(self, flow, step_name, environment):
        """
        pass

+    def add_to_package(self):


Can you provide an example on how this could be used?

pjoshi30 · 2021-11-24T20:14:53Z

metaflow/decorators.py

+    def add_to_package(self):
+        """
+        Called to add custom packages needed for a decorator.
+        Returns a list of tuples where each tuple represents (file_path,arcname)


please detail what file_path and arcname here represent

The MetaflowPackage class consists of a path_tuples function. This instance method helps seek the files to include for the code package. The path_tuples compiles the tarball of all the code files to include for execution. The function returns a list of tuples which consists of file_path and arcname; The filepath is the path of the file in the local FS to include and the arcname is the name/path of the file in the tarball when it is unpacked.

👍 thanks!

romain-intel · 2021-11-24T21:56:32Z

metaflow/package.py

+                    file_path, file_name = path_tuple
+                    # Check if the path is not duplicated as
+                    # many steps can have the same packages being imported
+                    check_name = "%s-%s" % (file_path, file_name)


Maybe I am being a bit too pedantic here but I would actually do something like this:

check_dict = {} if file_name not in check_dict or check_dict[file_name] == file_path: check_dict[file_name] = file_path yield path_tuple elif file_name in check_dict: # Raise some error saying that we already included a file for that path.

Yes this change seems much better than what I committed but I have a small alteration to propose to it:

if file_name not in deco_module_paths: deco_module_paths[file_name] = file_path yield path_tuple elif deco_module_paths[file_name] == file_path: pass elif file_name in deco_module_paths: # this means non unique file_name to file_path mapping. raise NonUniqueFileNameToFilePathMappingException( file_name, [deco_module_paths[file_name],file_path] )

With the change you suggested we yield path_tuple even when check_dict[file_name] == file_path which we don't want to do to avoid deduplication; Does that make sense ?

Also Piggbacking to what you said :

It also seems necessary to raise an error for this as it violates the uniqueness constraint of file_name to file_path mapping. There can be scenarios that people have a python package of metaflow-cards-scorecard installed via pip and the same is also present in the python path. In such cases we should fail with and error that file_name related to the following file_paths and needs to only have a unique file_name to file_path mapping.

Just a QQ:
What should be a name for this exception (Naming things is hard 😄 ) I have for now called it NonUniqueFileNameToFilePathMappingException

akyrola · 2021-12-03T19:58:31Z

metaflow/package.py

+                        yield path_tuple
+                    elif deco_module_paths[file_name] == file_path:
+                        pass
+                    elif (


should replace just with "else:"

With a just else we will throw an Exception even if deco_module_paths[file_name] == file_path (which we don't want to) . Rather I can change it to elif deco_module_paths[file_name] != file_path; this will keep logic the same

savingoyal · 2021-12-05T15:11:24Z

docs/lifecycle.dot

@@ -148,7 +149,8 @@ digraph Metaflow {
    /* package */
    validate_dag -> init_environment
    init_environment -> package_init
-    package_init -> add_to_package
+    package_init -> add_custom_package
+    add_custom_package -> add_to_package


@valayDave Can you also regenerate the new image?

romain-intel

Please fix:

Aapo's comment
the tiny format issue
regenerate the image
I will then merge.

romain-intel · 2021-12-06T21:38:38Z

metaflow/decorators.py

+        called in the `MetaflowPackage` class where metaflow compiles the code package
+        tarball. This hook is invoked in the `MetaflowPackage`'s `path_tuples`
+        function. The `path_tuples` function is a generator that yields a tuple of
+        `(file_path,arcname)`.`file_path` is the path of the file in the local file system;


nit: space after comma

romain-intel · 2021-12-06T21:38:54Z

metaflow/decorators.py

+    def add_to_package(self):
+        """
+        Called to add custom packages needed for a decorator.
+        Returns a list of tuples where each tuple represents (file_path,arcname)


romain-intel · 2021-12-06T21:39:57Z

metaflow/package.py

+                        yield path_tuple
+                    elif deco_module_paths[file_name] == file_path:
+                        pass
+                    elif (


- nit fixes (spaces after commas) - lifecycle PNG updated.

valayDave · 2021-12-07T18:17:02Z

@romain-intel : Made the changes. We are merge ready

add_to_package decorator hook.

8a6c972

- Cherry picking diff from card branch.

comment fix.

ad04b00

romain-intel reviewed Nov 23, 2021

View reviewed changes

supported including the same file as different name

892ea35

- Fixed comment.

pjoshi30 reviewed Nov 24, 2021

View reviewed changes

romain-intel reviewed Nov 24, 2021

View reviewed changes

valayDave mentioned this pull request Nov 26, 2021

@card Decorator [WIP] #815

Closed

13 tasks

valayDave added 3 commits November 28, 2021 18:18

Updated comment.

8434b85

updated comment with more context.

2714ec2

Refactoring logic for readability/correctness.

2bdc2e5

akyrola reviewed Dec 3, 2021

View reviewed changes

savingoyal reviewed Dec 5, 2021

View reviewed changes

romain-intel reviewed Dec 6, 2021

View reviewed changes

- Simplifying logic

2a3da50

- nit fixes (spaces after commas) - lifecycle PNG updated.

romain-intel approved these changes Dec 9, 2021

View reviewed changes

romain-intel merged commit 5305899 into Netflix:master Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`add_to_package` decorator hook. #834

`add_to_package` decorator hook. #834

valayDave commented Nov 23, 2021

valayDave commented Nov 23, 2021

valayDave commented Nov 23, 2021

romain-intel Nov 23, 2021

valayDave Nov 23, 2021

romain-intel Nov 23, 2021

skirdey Nov 23, 2021

valayDave Nov 24, 2021 •

edited

Loading

pjoshi30 Nov 24, 2021

valayDave Nov 28, 2021

pjoshi30 Nov 29, 2021

pjoshi30 Nov 24, 2021

pjoshi30 Nov 24, 2021

valayDave Nov 28, 2021

pjoshi30 Nov 29, 2021

romain-intel Dec 6, 2021

romain-intel Nov 24, 2021

valayDave Nov 28, 2021

akyrola Dec 3, 2021

romain-intel Dec 6, 2021

valayDave Dec 7, 2021

savingoyal Dec 5, 2021

valayDave Dec 7, 2021

romain-intel left a comment •

edited

Loading

romain-intel Dec 6, 2021

romain-intel Dec 6, 2021

romain-intel Dec 6, 2021

valayDave commented Dec 7, 2021

add_to_package decorator hook. #834

add_to_package decorator hook. #834

Conversation

valayDave commented Nov 23, 2021

valayDave commented Nov 23, 2021

valayDave commented Nov 23, 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

valayDave Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Context

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Why do we use this hook

Example of metaflow_cards Namespace package.

Gotacha's with directly seeking Installed Modules

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

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

romain-intel left a comment • edited Loading

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

valayDave commented Dec 7, 2021

`add_to_package` decorator hook. #834

`add_to_package` decorator hook. #834

valayDave Nov 24, 2021 •

edited

Loading

Example of `metaflow_cards` Namespace package.

romain-intel left a comment •

edited

Loading