Option to return proto OR dict for SeldonClient methods #1175
Conversation
…for Seldon Client functions
axsaucedo
changed the title
|
/cc @cliveseldon @adriangonz |
There was a problem hiding this comment.
Besides the small comment (where it may not even be necessary to change anything), I think it looks good!
As an aside, something out of the scope of this PR, I think that SeldonClient may have too many parameters now? I'm not sure if this is the case, and even if it is, I'm not sure if it would even be an issue.
| assert "jsonData" in mock_post.call_args[1]["json"] | ||
| assert mock_post.call_args[1]["json"]["jsonData"] == JSON_TEST_DATA | ||
| assert response.success is True | ||
| assert json_response["jsonData"] == JSON_TEST_DATA |
There was a problem hiding this comment.
Perhaps it would be good to add an assertion here for having a dict type? I think something like this should work:
assert isinstance(json_response, dict)Similarly, we could add assertions on the existing tests for SeldonMessage?
PS: I just realised that the SeldonMessage ones use seldon_message_to_json(...) so perhaps this is not necessary.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriangonz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Added functionality to specify proto return type or json return type for Seldon Client functions
Fixes #1083