-
Notifications
You must be signed in to change notification settings - Fork 828
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
Nodejs s2i wrapper for JavaScript models #218
Conversation
Also while I was at it, I tried deploying an R model. I found that the s2i environment required as per the R wrapper s2i env docs doesn't work. The parameter model name has to have the file extension also to make it work. For instance,in the s2i/environment file, Should I add this minor change as a part of this request or create a separate one? |
Also fixing the issue in the R wrapper documentation as per issue #219 |
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.
Running the steps in the Readme after installing nodejs I get an error on the last part npm run predict:
2018-09-09 17:42:51.698991: I tensorflow/core/platform/cpu_feature_guard.cc:141] Your CPU supports instructions that this TensorFlow binary was not compiled to use: SSE4.1 SSE4.2 AVX AVX2 FMA
(node:32104) Warning: N-API is an experimental feature and could change at any time.
Predicting ...
(node:32104) UnhandledPromiseRejectionWarning: TypeError: predict.print is not a function
at run (/home/clive/work/seldon-core/fork-seldon-core/examples/models/nodejs_tensorflow/predict.js:18:11)
at <anonymous>
(node:32104) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:32104) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Can you also add more documentation for the example explaining what it will do. Also, said a notebook for the steps would follow the existing examples. |
@cliveseldon I have made some necessary changes including the explanation of the model example. Can you please review the update. Thanks a lot! |
The changes look good. So I think what is still needed is:
|
@cliveseldon I have added the makefiles for both the example model and the wrapper. You may review them. |
@cliveseldon I have added the ipython notebook. Let me know if something else can be improved. |
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.
It would be ideal to have a real example based on a standard dataset. So forexample, the Iris data set or MNIST. There are other examples that use these. Are there not any examples that people have provided for tensorflow_js that can easily be used?
|
||
|
||
.PHONY: test | ||
test: |
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.
make test does not work:
make test
docker build -t docker.io/seldonio/seldon-core-s2i-nodejs:0.1-candidate .
Sending build context to Docker daemon 25.6kB
Step 1/9 : FROM node:latest
---> d0d12094f6ab
Step 2/9 : LABEL io.openshift.s2i.scripts-url="image:///s2i/bin"
---> Using cache
---> aaef67401575
Step 3/9 : RUN mkdir microservice
---> Using cache
---> 6cfa93b61811
Step 4/9 : WORKDIR /microservice
---> Using cache
---> 4dffc6d86792
Step 5/9 : COPY microservice.js /microservice
---> Using cache
---> 318f520dc0a0
Step 6/9 : COPY package.json /microservice
---> Using cache
---> 42dcbdf715b9
Step 7/9 : RUN npm install
---> Using cache
---> 9f165e8ab34b
Step 8/9 : COPY ./s2i/bin/ /s2i/bin
---> Using cache
---> 8f075578815b
Step 9/9 : EXPOSE 5000
---> Using cache
---> bd7b6e6f5275
Successfully built bd7b6e6f5275
Successfully tagged seldonio/seldon-core-s2i-nodejs:0.1-candidate
IMAGE_NAME=docker.io/seldonio/seldon-core-s2i-nodejs:0.1-candidate test/run
/bin/sh: 1: test/run: not found
Makefile:17: recipe for target 'test' failed
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.
The recipe failed because it couldn't find the appropriate run file within the test folder which is present now. So this should be working now. Please review. Thanks.
You seem to have an editor which is changing all the markdown from |
Ok, this is done. Can you confirm? |
wrappers/s2i/nodejs/package.json
Outdated
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"author": "SachinVarghese", | ||
"license": "ISC", |
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 need to change this to Apache 2 to fit with Seldon Core license
wrappers/s2i/nodejs/microservice.js
Outdated
args.service, | ||
args.persistence | ||
); | ||
console.log("NodeJs Microservice listening on port 5000!"); |
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.
port is hardwired here.
wrappers/s2i/nodejs/microservice.js
Outdated
result = array_to_rest_data(result, body); | ||
res.status(200).send(result); | ||
} else { | ||
res.status(500).send(predict); |
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.
Please add console messages as well on error.
wrappers/s2i/nodejs/microservice.js
Outdated
body = JSON.parse(req.body.json); | ||
body = body.data; | ||
} catch (msg) { | ||
res.status(500).send("Cannot parse predict input json " + req.body.json); |
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.
Please add console message as well on error.
Thanks for all your hard work on this. I think this is ready to merge. |
Thanks for your patience and testing multiple times. I will try to make it easier for you next time around. |
Creating a pull request for the implementation of Nodejs s2i wrapper for JavaScript models as per issue #216 .