-
Notifications
You must be signed in to change notification settings - Fork 817
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
KServe wrapper default configuration is faulty #2956
Comments
Hi @sgaist Thanks for the details! If you can add a test to this, then it would easy to understand and expedite your PR |
Hi @agunapal, Thanks for the pointer ! I ran the tests and while checking how they were going on, I saw that the Seeing that, I verified what I have on my current machine and surprisingly, it's an older version, v0.9.0 to be exact, of the KServe controller that has been deployed. I don't know why that version as I have followed their documentation to deploy the whole system. This means that the current version of KServe does the right thing when creating the deployment from While writing the test, I saw some things that could be improved such as the configuration default handling. I am wondering whether such improvements should be part of a follow up patch or integrated in this one ? |
Hi @sgaist The nightly tests that we run are on nightly images. Hence, you don't see the issue I guess. We will be be making a new release soon. |
Hi @sgaist We welcome any improvements to the code. You can send a PR for one of the issues you want to fix, include a test and we can take it from there |
Good, then I'll open a ticket about the improvements I have in mind for the wrapper on top of what I have done in this one with a follow up patch. |
馃悰 Describe the bug
The
TorchserveModel
class is not initialized correctly when called from__main__.py
.The class initializes the
protocol
property with whatever is passed as parameters.The issue lies in the handling of the
PROTOCOL_VERSION
environment variable. The default value will beNone
if not set and thus theprotocol
property will be as well. In the base class implementation, if no configuration is passed, the default value isv1
.This become an issue when the answer is generated as the protocol version is checked.
Error logs
Installation instructions
The installation procedure does not matter.
The issue manifests itself in both the Docker container and with a source or package installation
Model Packaing
torch-model-archiver --model-name test --version 1.0 --export-path model-store --handler handler.py -f
config.properties
Versions
Environment headers
Torchserve branch: HEAD
**Warning: torchserve not installed ..
**Warning: torch-model-archiver not installed ..
Python version: 3.9 (64-bit runtime)
Python executable: ~/miniconda3/envs/pytorchserve-dev/bin/python
Versions of relevant python libraries:
numpy==1.26.4
psutil==5.9.8
requests==2.31.0
requests-oauthlib==1.3.1
wheel==0.41.2
**Warning: torch not present ..
**Warning: torchtext not present ..
**Warning: torchvision not present ..
**Warning: torchaudio not present ..
Java Version:
OS: Mac OSX 13.6.4 (arm64)
GCC version: N/A
Clang version: 15.0.0 (clang-1500.1.0.2.5)
CMake version: version 3.28.3
Versions of npm installed packages:
**Warning: newman, newman-reporter-html markdown-link-check not installed...
Repro instructions
create environment with
serve
installedcreate
handler.py
from sample codecreate
config.properties
from exampletorch-model-archiver --model-name test --version 1.0 --export-path model-store --handler handler.py -f
create environment with kserve installed
clone this repo
create
/mnt/models/config/config.properties
with:start kserve wrapper:
create payload
data.json
:send request
Possible Solution
There are several:
PROTOCOL_VERSION
environment variableNone
in the__init__
function and set it tov1
None
in the__init__
function and do not set it so it keeps the default value of the base classIn the idea of failing early, adding a check for the validity of the value would likely also be worthwhile as
PROTOCOL_VERSION
might contain an invalid value.The text was updated successfully, but these errors were encountered: