-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
migrated base runtime to core #380
Conversation
Documentation preview |
@@ -23,6 +23,7 @@ | |||
from google.protobuf import text_format | |||
|
|||
from merlin.dag import postorder_iter_nodes | |||
from merlin.dag.base_runtime import Runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the convention where the names of classes and packages start with "base." We were kind of painted into a corner with Operator
vs BaseOperator
, since Operator
already existed and historically was the base class of all operators, but I'm thinking maybe the subpackage in Core should be called runtimes
so this reads as from merlin.dag.runtimes import Runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You good with making this change in a subsequent PR? If not we need to merge NVIDIA-Merlin/core#359 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do 359 first
…rm-base-runtime
This PR requires core PR: NVIDIA-Merlin/core#358
We are migrating the base runtime to the core library. This is done in an effort to consolidate how graphs are executed. We want to move the framework to where both nvtabular workflows and ensembles use runtimes. This will allow us to simplify the framework to only use one mechanism for execution.