Conversation
|
Test FAILed. |
|
Test FAILed. |
dcrankshaw
left a comment
There was a problem hiding this comment.
This looks like it was a pain to write, but the end result is great!
| return(deserialized_pred_fn) | ||
| } | ||
| input_class = class(deserialized_input) | ||
| if(input_class != input_class) { |
There was a problem hiding this comment.
This looks like a hard comparison to fail...
There was a problem hiding this comment.
Fixed. We now correctly compare the class of the deserialized input received by the container to the class of inputs expected to be received by the prediction function. These should match.
|
During code review, we came up with a few different design choices for how the client-side model deployment package should work.
|
969b866 to
ff8b879
Compare
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
dcrankshaw
left a comment
There was a problem hiding this comment.
Cool. Looks good. Add tests and update the Docker file and I'll try it out.
containers/R/RContainerDockerfile
Outdated
| @@ -0,0 +1,24 @@ | |||
| FROM r-base | |||
There was a problem hiding this comment.
Add the following two lines above the FROM
# This ARG isn't used but prevents warnings in the build script
ARG CODE_VERSION
containers/R/RContainerDockerfile
Outdated
| RUN R CMD INSTALL rclipper_serve | ||
|
|
||
| ENTRYPOINT ["/entrypoint.sh"] | ||
|
|
There was a problem hiding this comment.
Move this to the dockerfiles/ subdirectory and add a call to build it to bin/build_docker_images.sh
containers/R/RContainerDockerfile
Outdated
|
|
||
| COPY rclipper_serve rclipper_serve | ||
| COPY serve_model.R serve_model.R | ||
| COPY r_container_entrypoint.sh entrypoint.sh |
There was a problem hiding this comment.
Don't change the name of the file when you copy it (just to make debugging a little easier)
containers/R/RContainerDockerfile
Outdated
| ENV CLIPPER_MODEL_PATH=/model | ||
| ENV CLIPPER_PORT=7000 | ||
|
|
||
| RUN R -e "install.packages('jsonlite', repos='http://cran.us.r-project.org')" |
There was a problem hiding this comment.
Combine these into a single RUN command
| Version: 1.0 | ||
| Date: 2017-07-15 | ||
| Author: Corey Zumar | ||
| Maintainer: Dan Crankshaw <crankshaw@eecs.berkeley.edu> |
There was a problem hiding this comment.
Make yourself the maintainer
| Version: 1.0 | ||
| Date: 2017-07-22 | ||
| Author: Corey Zumar | ||
| Maintainer: Dan Crankshaw <crankshaw@eecs.berkeley.edu> |
There was a problem hiding this comment.
Make yourself maintainer
| Package: rclipper | ||
| Type: Package | ||
| Title: Containerizes and deploys R closures to Clipper | ||
| Version: 1.0 |
| @@ -0,0 +1,206 @@ | |||
| get_library_dependencies = function(cd_dependencies) { | |||
There was a problem hiding this comment.
This should definitely have some tests
| @@ -0,0 +1,76 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Update this file to match the latest version of clipper_admin.
|
|
||
| print("Building image and deploying model...") | ||
|
|
||
| cl_conn.deploy_model(args.model_name, args.model_version, input_type, |
There was a problem hiding this comment.
This should be build_model. As discussed, let's just build the model in this script and not deploy it.
5478518 to
e2089ba
Compare
|
Test FAILed. |
Corey-Zumar
left a comment
There was a problem hiding this comment.
Addressed most of the review comments. This still needs unit tests and documentation. I also still need to include the R container integration test in our integration test suite.
containers/R/RContainerDockerfile
Outdated
| @@ -0,0 +1,24 @@ | |||
| FROM r-base | |||
| Package: rclipper | ||
| Type: Package | ||
| Title: Containerizes and deploys R closures to Clipper | ||
| Version: 1.0 |
| Version: 1.0 | ||
| Date: 2017-07-22 | ||
| Author: Corey Zumar | ||
| Maintainer: Dan Crankshaw <crankshaw@eecs.berkeley.edu> |
containers/R/RContainerDockerfile
Outdated
|
|
||
| COPY rclipper_serve rclipper_serve | ||
| COPY serve_model.R serve_model.R | ||
| COPY r_container_entrypoint.sh entrypoint.sh |
containers/R/RContainerDockerfile
Outdated
| ENV CLIPPER_MODEL_PATH=/model | ||
| ENV CLIPPER_PORT=7000 | ||
|
|
||
| RUN R -e "install.packages('jsonlite', repos='http://cran.us.r-project.org')" |
containers/R/RContainerDockerfile
Outdated
| RUN R CMD INSTALL rclipper_serve | ||
|
|
||
| ENTRYPOINT ["/entrypoint.sh"] | ||
|
|
| return(deserialized_pred_fn) | ||
| } | ||
| input_class = class(deserialized_input) | ||
| if(input_class != input_class) { |
There was a problem hiding this comment.
Fixed. We now correctly compare the class of the deserialized input received by the container to the class of inputs expected to be received by the prediction function. These should match.
| if(input_class != input_class) { | ||
| return(sprintf("Received invalid input of class `%s`` for model expecting inputs of class `%s`", | ||
| input_class, | ||
| input_class)) |
|
Test FAILed. |
|
Test PASSed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
i killed the job as the worker was inundated w/zombie procs. i'll kick off a new one now. |
|
test this please |
|
jenkins test this please |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Jenkins test this please |
|
Test FAILed. |
|
Test PASSed. |
dcrankshaw
left a comment
There was a problem hiding this comment.
Fixed a final bug where Rcpp::NumericVector is equivalent to std::vector<double> not std::vector<float>. Once the tests pass with this change, this is good to go.
|
Test PASSed. |
|
Awesome! Thanks Dan! |
Note: This depends on #251. Rebasing after it is merged will significantly reduce the file diff. This is functional, but it still needs documentation.