-
Notifications
You must be signed in to change notification settings - Fork 30
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
Load models from a model registry rather than path #149
Load models from a model registry rather than path #149
Conversation
Click to view CI ResultsGitHub pull request #149 of commit 7525fc31a4dead4371890b60c719c5efc8c6ec4e, no merge conflicts. Running as SYSTEM Setting status of 7525fc31a4dead4371890b60c719c5efc8c6ec4e to PENDING with url https://10.20.13.93:8080/job/merlin_systems/152/console and message: 'Pending' Using context: Jenkins Building on master in workspace /var/jenkins_home/workspace/merlin_systems using credential fce1c729-5d7c-48e8-90cb-b0c314b1076e > git rev-parse --is-inside-work-tree # timeout=10 Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/NVIDIA-Merlin/systems # timeout=10 Fetching upstream changes from https://github.com/NVIDIA-Merlin/systems > git --version # timeout=10 using GIT_ASKPASS to set credentials login for merlin-systems user + githubtoken > git fetch --tags --force --progress -- https://github.com/NVIDIA-Merlin/systems +refs/pull/149/*:refs/remotes/origin/pr/149/* # timeout=10 > git rev-parse 7525fc31a4dead4371890b60c719c5efc8c6ec4e^{commit} # timeout=10 Checking out Revision 7525fc31a4dead4371890b60c719c5efc8c6ec4e (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 7525fc31a4dead4371890b60c719c5efc8c6ec4e # timeout=10 Commit message: "remove implementation classes from tests" > git rev-list --no-walk 9910bfe09817c83159a425874220a69c59878d57 # timeout=10 [merlin_systems] $ /bin/bash /tmp/jenkins5494403820059294037.sh PYTHONPATH=:/usr/local/lib/python3.8/dist-packages/:/usr/local/hugectr/lib:/var/jenkins_home/workspace/merlin_systems/systems ============================= test session starts ============================== platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0 rootdir: /var/jenkins_home/workspace/merlin_systems/systems, configfile: pyproject.toml plugins: anyio-3.6.1, xdist-2.5.0, forked-1.4.0, cov-3.0.0 collected 51 items |
Documentation preview |
Click to view CI ResultsGitHub pull request #149 of commit 3f7534853b39af8d7042e05e5d378d390948c92d, no merge conflicts. Running as SYSTEM Setting status of 3f7534853b39af8d7042e05e5d378d390948c92d to PENDING with url https://10.20.13.93:8080/job/merlin_systems/153/console and message: 'Pending' Using context: Jenkins Building on master in workspace /var/jenkins_home/workspace/merlin_systems using credential fce1c729-5d7c-48e8-90cb-b0c314b1076e > git rev-parse --is-inside-work-tree # timeout=10 Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/NVIDIA-Merlin/systems # timeout=10 Fetching upstream changes from https://github.com/NVIDIA-Merlin/systems > git --version # timeout=10 using GIT_ASKPASS to set credentials login for merlin-systems user + githubtoken > git fetch --tags --force --progress -- https://github.com/NVIDIA-Merlin/systems +refs/pull/149/*:refs/remotes/origin/pr/149/* # timeout=10 > git rev-parse 3f7534853b39af8d7042e05e5d378d390948c92d^{commit} # timeout=10 Checking out Revision 3f7534853b39af8d7042e05e5d378d390948c92d (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 3f7534853b39af8d7042e05e5d378d390948c92d # timeout=10 Commit message: "fix typo" > git rev-list --no-walk 7525fc31a4dead4371890b60c719c5efc8c6ec4e # timeout=10 [merlin_systems] $ /bin/bash /tmp/jenkins9378471006788988482.sh PYTHONPATH=:/usr/local/lib/python3.8/dist-packages/:/usr/local/hugectr/lib:/var/jenkins_home/workspace/merlin_systems/systems ============================= test session starts ============================== platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0 rootdir: /var/jenkins_home/workspace/merlin_systems/systems, configfile: pyproject.toml plugins: anyio-3.6.1, xdist-2.5.0, forked-1.4.0, cov-3.0.0 collected 51 items |
merlin/systems/dag/ops/operator.py
Outdated
|
||
# validate that we have a model_or_path argument. This is a total hack and perhaps a smell | ||
# that all inference operators should accept a model_or_path, or at least we introduce | ||
# another layer of base class that does. |
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.
One trick I use sometimes when there are two options is to make one of them the type expected by the constructor and then add a class method that takes the other type and converts it. In this case, I'd probably pass the model type into constructors, and add a class method like from_path
that loads the models and passes them to the constructors.
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.
That's a great idea. In the case of TransformWorkflow
the from_path
method could throw some kind of exception since it doesn't really make sense for that operator.
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 think it would make sense if you want to load a Workflow from a file, which is not an uncommon thing to do.
merlin/systems/model_registry.py
Outdated
import requests | ||
|
||
|
||
class ModelRegistry: |
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.
Since this only has an abstract method, maybe the class should be abstract too?
Click to view CI ResultsGitHub pull request #149 of commit a07858a826a9a11109436ae3cd16701876e6df9c, no merge conflicts. Running as SYSTEM Setting status of a07858a826a9a11109436ae3cd16701876e6df9c to PENDING with url https://10.20.13.93:8080/job/merlin_systems/154/console and message: 'Pending' Using context: Jenkins Building on master in workspace /var/jenkins_home/workspace/merlin_systems using credential fce1c729-5d7c-48e8-90cb-b0c314b1076e > git rev-parse --is-inside-work-tree # timeout=10 Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/NVIDIA-Merlin/systems # timeout=10 Fetching upstream changes from https://github.com/NVIDIA-Merlin/systems > git --version # timeout=10 using GIT_ASKPASS to set credentials login for merlin-systems user + githubtoken > git fetch --tags --force --progress -- https://github.com/NVIDIA-Merlin/systems +refs/pull/149/*:refs/remotes/origin/pr/149/* # timeout=10 > git rev-parse a07858a826a9a11109436ae3cd16701876e6df9c^{commit} # timeout=10 Checking out Revision a07858a826a9a11109436ae3cd16701876e6df9c (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f a07858a826a9a11109436ae3cd16701876e6df9c # timeout=10 Commit message: "refactor to add from_path method" > git rev-list --no-walk 3f7534853b39af8d7042e05e5d378d390948c92d # timeout=10 [merlin_systems] $ /bin/bash /tmp/jenkins17333612293791564833.sh PYTHONPATH=:/usr/local/lib/python3.8/dist-packages/:/usr/local/hugectr/lib:/var/jenkins_home/workspace/merlin_systems/systems ============================= test session starts ============================== platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0 rootdir: /var/jenkins_home/workspace/merlin_systems/systems, configfile: pyproject.toml plugins: anyio-3.6.1, xdist-2.5.0, forked-1.4.0, cov-3.0.0 collected 50 items |
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.
🚀
This gives the option to load models into
PredictTensorflow
from any model registry, and includes an implementation for mlflow.Example usage:
The implementation simply gets the path to the model and uses it to call
PredictTensorflow
's init method.One issue that I ran into is that it seems like it would be nice to have this available for all implementations of
InferenceOperator
, but not all of those implementations accept a model path as an input - in fact onlyPredictTensorflow
does.FIL
only takes a model input as an input, andTransformWorkflow
doesn't use a model at all.I came up with a questionable solution of looking for
model_or_path
in the signature of the implementation class's__init__
method (in fact, it should be the first argument to that method). I'd love to hear a suggestion of a better way to do this.