Use Runtimes and Executors to serve NVT Workflows
#320
Conversation
The workflow output format should now be the same for both TF and Torch, so we can condense their respective sub-classes into the base `WorkflowRunner`, which only leaves the HugeCTR sub-class to deal with later (when we have HugeCTR support.) Move `_convert_to_np` to the HugeCTR `WorkflowRunner`
Documentation preview |
karlhigley
force-pushed
the
refactor/runtimes-executors
branch
from
6ac2f00 to
9789068
Compare
| @@ -44,12 +68,17 @@ def transform(self, graph: Graph, transformable: Transformable): | |||
| Graph of nodes container operator chains for data manipulation. | |||
| transformable : Transformable | |||
| Input data to transform in graph. | |||
| convert: bool | |||
There was a problem hiding this comment.
what's the scenario where we'd want to pass convert=False?
There was a problem hiding this comment.
The split between convert and transform allows you to do the conversion ahead of time manually and then pass convert=False in order to avoid doing it every time the transform runs as a small optimization. We put this in so that it was possible to maintain the previous behavior where the conversion only happens once, while also offering the convenience of doing it automatically in the cases where you're not worried about the perf implications.
|
|
||
| return formatted_tensors | ||
|
|
||
|
|
There was a problem hiding this comment.
Note that pretty much everything in this file from here down is just getting moved from other places. There's definitely more refactoring we want to do on the code below, but moving it here more clearly communicates that the usage of this code is isolated to this single file and can be freely refactored without causing issues elsewhere.
| raise TypeError(f"Unknown type: {type(data)}") | ||
|
|
||
|
|
||
| def _convert_format(tensors, target_format): |
There was a problem hiding this comment.
A next step along the road of this refactor is to replace DictArray with TensorTable and get rid of the manual conversions below. I think we'll still need a function like _convert_format, but the actual conversions it uses should be the dataframe/TensorTable conversions from Merlin Core.
| tensors = self._standardize_formats(workflow_node, upstream_outputs) | ||
|
|
||
| transform_input = _concat_tensors(tensors) | ||
| # TODO: In order to replace the line above with the line below, we first have to replace |
There was a problem hiding this comment.
This is one difference from the LocalExecutor._execute_node. The other seems to be the second line calling _merge_addl_root_columns instead of _append_addl_root_columns. Is that something that we might be able to unify with LocalExecutor eventually too?
There was a problem hiding this comment.
Yeah, we're aiming to address those things in the next PR in this series of changes. We split the PRs here because fixing the TODO above ended up requiring some deeper changes that run through Core and several of the other libraries.
| LOG = logging.getLogger("merlin-systems") | ||
|
|
||
|
|
||
| class NVTabularServingExecutor(LocalExecutor): |
There was a problem hiding this comment.
Is the idea that we're working toward unifying the functionality required here into LocalExecutor entirely?
There was a problem hiding this comment.
As much as possible, yes. The ideal end state would be getting rid of this executor entirely 👍🏻
This reworks the existing NVTabular Workflow serving to use some of the newer concepts we've introduced, while continuing to support the optimized C++ implementations of NVT ops for serving:
LocalExecutorthat contains the custom code for running Workflows at serving time (which involves some additional conversion between data formats)Runtimethat contains the appropriate operator implementations substitutionsOpTableabstraction in order to encapsulate logic about when ops should be substitutedDepends on NVIDIA-Merlin/core#279