-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
BUG: fixed model serve fail with HTTP 400 on Bad Request. #5003
Conversation
f4a7ac5
to
c052b33
Compare
@abatomunkuev Thanks for the contribution! The DCO check failed. Please sign off your commits by following the instructions here: https://github.com/mlflow/mlflow/runs/4102308049. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.rst#sign-your-work for more details. |
fdb9b38
to
a6340c8
Compare
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
a6340c8
to
468f390
Compare
Hi @abatomunkuev, the failures in https://github.com/mlflow/mlflow/runs/4104942369?check_suite_focus=true appear to be caused by test cases asserting that the response of certain scoring operations with bad input is The failures in https://github.com/mlflow/mlflow/runs/4104942145?check_suite_focus=true appear to be unrelated. I'm investigating the cause. |
The |
@dbczumar Hello! I am going to work on updating the tests. |
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
@dbczumar Hello! I have made some changes to the following test case in mlflow/tests/models/test_cli.py Line 468 in 0f9d1e0
I wondering should we update the following line? mlflow/tests/models/test_cli.py Line 494 in 0f9d1e0
Should we change |
Hi @abatomunkuev . Yes, let's change MALFORMED_REQUEST to BAD_REQUEST. |
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
7feba10
to
18e0d2c
Compare
@dbczumar done! |
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.
LGTM! Thanks @abatomunkuev !
@abatomunkuev Almost there! Looks like |
@abatomunkuev Apologies for misinterpreting the test output. It appears that the server is still responding with 500s where 400s are now expected. |
@dbczumar Should we update the error codes in In some functions we are still returning I am not sure if its a good way to solve the issue. We can either update |
@abatomunkuev I think it's best to change other instances to |
Okay, I am going to update the code in |
Thank you! |
@abatomunkuev Looks like there are a few more small failures in test cases that expect |
@dbczumar Sure, I just need to find which test cases fail. |
@dbczumar Thanks! |
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
ceca430
to
0a549e3
Compare
@abatomunkuev Perhaps there's something special about the empty string case being tested here? |
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
63361b8
to
97d3a71
Compare
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
@dbczumar Could you review the changes? I hope this time it will work. I am wondering if we should leave |
Thanks @abatomunkuev. Your change looks good - let's get rid of |
@dbczumar Sure! |
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
@abatomunkuev Can we resolve conflicts with the master branch for |
@dbczumar Yes. What do I need to do to resolve this conflict? |
Signed-off-by: dbczumar <corey.zumar@databricks.com>
@dbczumar I think this is portion of code causes the error. I don't know why it causes the error, since we changed all Or maybe we should update the value of the |
@abatomunkuev Let me know if you need help investigating this. |
@dbczumar I would like to run tests locally. However, I got the error building a docker image to serve the model. I'm running a test: |
I am going to update the value of the
|
Signed-off-by: Andrei Batomunkuev <abatomunkuev@myseneca.ca>
@abatomunkuev Did we identify an issue with the empty string? |
@dbczumar Not complete sure, but it might be the cause. Also, I have a question. How can I test the cases that involves Docker containers & serving the model on my local machine. I am not able to do that, it gives me the error. |
@abatomunkuev Do you have Docker installed on your machine? |
@dbczumar Yes, I do have. |
Signed-off-by: harupy <hkawamura0130@gmail.com>
Signed-off-by: harupy <hkawamura0130@gmail.com>
@dbczumar @abatomunkuev I pushed a commit to fix the test failures: 4308be6 It looks like |
Fantastic! Thank you so much for your contribution, @abatomunkuev , and thank you for identifying the cause of the failure, @harupy ! |
Signed-off-by: Andrei Batomunkuev andreibatomunkuev@gmail.com
What changes are proposed in this pull request?
Solves #4897.
Please refer to my root cause analysis of the issue. I have explained that the root cause of the issue might be an incorrect passing value
MALFORMED REQUEST
to the argumenterror_code
in function_handle_serving_error
.To fix this, I have changed the passing value argument of
error_code
fromMALFORMED REQUEST
toBAD REQUEST
.The HyperText Transfer Protocol (HTTP) 500 Internal Server Error server error response code indicates that the server encountered an unexpected condition that prevented it from fulfilling the request. Source
The HyperText Transfer Protocol (HTTP) 400 Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing). Source
To reproduce the error lets run the following command from the issue
In our case, the error should be
400 Bad Request
since we are passing an incorrect syntax/value of json object. So, the error message should reflect the corresponding issue.How is this patch tested?
Currently, the unit tests
test_scoring_server.py
fail since we have updated the value toBAD REQUEST
. So, we need to either fix those existing tests or write a new unit test. I will need some help in fixing unit tests, I am a little bit confused which unit test belong to this issue.Release Notes
Is this a user-facing change?
When the users passes an incorrect syntax of json object to the server, the server should display the following message:
Example of the incorrect json syntax value:
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts
: Artifact stores and artifact loggingarea/build
: Build and test infrastructure for MLflowarea/docs
: MLflow documentation pagesarea/examples
: Example codearea/model-registry
: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models
: MLmodel format, model serialization/deserialization, flavorsarea/projects
: MLproject format, project running backendsarea/scoring
: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra
: MLflow Tracking server backendarea/tracking
: Tracking Service, tracking client APIs, autologgingInterface
area/uiux
: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker
: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy
: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows
: Windows supportLanguage
language/r
: R APIs and clientslanguage/java
: Java APIs and clientslanguage/new
: Proposals for new client languagesIntegrations
integrations/azure
: Azure and Azure ML integrationsintegrations/sagemaker
: SageMaker integrationsintegrations/databricks
: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change
- The PR will be mentioned in the "Breaking Changes" sectionrn/none
- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature
- A new user-facing feature worth mentioning in the release notesrn/bug-fix
- A user-facing bug fix worth mentioning in the release notesrn/documentation
- A user-facing documentation change worth mentioning in the release notes