Do not raise if error_code is missing. Avoid dropping response body o… #1192
Conversation
mlflow/exceptions.py
Outdated
| @@ -37,7 +37,7 @@ def serialize_as_json(self): | |||
| class RestException(MlflowException): | |||
| """Exception thrown on non 200-level responses from the REST API""" | |||
| def __init__(self, json): | |||
| error_code = json['error_code'] | |||
| error_code = json.get('error_code') | |||
There was a problem hiding this comment.
Did you mean for this to be json.get('error_code', INTERNAL_ERROR) ?
There was a problem hiding this comment.
Currently I left the default value to MlflowException's default value( None -> INTERNAL_ERROR). I can make it explicit or, if there is a better error_code, we can set it here as you described.
There was a problem hiding this comment.
I think both options are fine.
One difference is, if there is no message nor error_code in the json then the exception will have no message.
|
@aarondav for thoughts |
mlflow/exceptions.py
Outdated
| @@ -37,7 +37,7 @@ def serialize_as_json(self): | |||
| class RestException(MlflowException): | |||
| """Exception thrown on non 200-level responses from the REST API""" | |||
| def __init__(self, json): | |||
| error_code = json['error_code'] | |||
| error_code = json.get('error_code') | |||
There was a problem hiding this comment.
I think both options are fine.
One difference is, if there is no message nor error_code in the json then the exception will have no message.
|
LGTM - will merge after Travis passes! |
If error_code is missing in the exception response, an KeyError: 'error_code' is raised. This drops the error message completely.
What changes are proposed in this pull request?
Use get instead of accessing the key directly for error_code in response json. This avoids a KeyError for a missing "error_code" key in new server implementations. It may be worth introducing a specific default, for now the PR proposes InternalError as the default error_code which is the current default.
How is this patch tested?
A test, using the new class omitting the "error_code" top level property was added.
(Details)
Release Notes
Is this a user-facing change?
(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
What component(s) does this PR affect?
How 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