Add support for serializing modules involved in LambdaOp execution by value #1741
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds an optional parameter to Workflow.save so that users can indicate that certain modules should be serialized by value. This is necessary if a LambdaOp in a workflow depends on a module whose source file will not be available in the deployment environment. Related to NVIDIA-Merlin#1737.
This commit adds code to automatically infer LambdaOp module dependencies in several common cases: 1. in which a function is passed to LambdaOp by name, 2. in which the argument to LambdaOp is a lambda expression that refers to a function by a fully-qualified name, and 3. in which the argument to LambdaOp is a lambda expression that refers to a function via another variable The current implementation does not inspect the bodies of any function passed to LambdaOp, and many corner cases are (necessarily) omitted. However, this support should be complete enough to be useful for many users. Automatic inference is optional (via a parameter to Workflow.save) but it could be the default in the future. Related to issue NVIDIA-Merlin#1737.
karlhigley
approved these changes
Dec 27, 2022
Documentation preview |
|
The CI tests on this PR appear to hang during |
Thanks, @karlhigley — I’ll try and figure out what’s going on here. |
|
Thanks, @karlhigley! |
|
Thank you, @willb! Sorry it took us so long to get it merged, but this looks great. We literally ran into this problem again today, so the timing is fortuitous. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These commits address issue #1737 in two ways:
Workflow.saveto serialize an explicit list of named modules by value, andWorkflow.saveto use a heuristic to automatically infer the external modules involved in a workflow.Taken together, these commits will make it easier to serialize workflows that are resilient to execution in environments without source files for all of the modules that were available when the workflow was created, e.g., Docker containers.
The main contribution is the addition of a
modules_byvalueparameter toWorkflow.save. If this is passed an explicit list of modules,Workflow.savewill directcloudpickleto serialize these modules by value. If it is passed the string"auto",Workflow.savewill employ a heuristic approach that will be useful for many real-world uses of LambdaOp.I believe that it would be harmless and useful to make
"auto"the default but have not done so in this PR.For more background on some of the tradeoffs that would be involved in making the heuristic more precise, please see this blog post. I do not believe that many of the cases I identified in that post (explicit imports within functions, list comprehensions, etc.) are likely to present in realistic NVTabular LambdaOp client code, and thus took a relatively simple approach.