WIP: Python wrappers rewrite #457
Conversation
There was a problem hiding this comment.
A few points:
- Add
pytest-covtotests_requireinsetup.pyand the pytest flag--cov=seldon_coreinsetup.cfgunderaddoptsto get test coverage report. - Expose
SeldonClientat top level of package max_grpc_msg_size.ipynbconfusingly deletesseldon-core(!helm delete seldon-core --purge) between two examples which causes the notebook not to work end-to-end- Add a note for
max_grpc_msg_size.ipynbto run the notebook withjupyter notebook --NotebookApp.iopub_data_rate_limit=1000000000for it to run successfully
There was a problem hiding this comment.
I haven't found any serious problems. Left some comments, mostly formatting and style, a couple of suggestions for improvements and minor issues. Unfortunately, I didn't have time to pull the branch and inspect it in IDE yet, only checked on GitHub quickly. If you can wait, I'll have a bit more time on Friday. Otherwise, LGTM from me. Thank you for this rewrite!
| def main(): | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("contract", type=str, | ||
| help="File that contains the data contract") |
There was a problem hiding this comment.
We wrote a script to generate the contract automatically based on a dataset sample in pandas.DataFrame, let us know if you want it.
There was a problem hiding this comment.
That would be very useful!
There was a problem hiding this comment.
Great! How it's better proceed with it? Should I create a PR or just pass the code to you? I need to clean up a bit. Will be ready on Monday.
There was a problem hiding this comment.
A PR would be great.
|
I can't see this branch when I do |
|
Sorry, I didn't realise Clive was merging from his fork of seldon-core. Never mind my question. Clear now. |
|
Shouldn't all API testing functionality go into a separate package inside |
There was a problem hiding this comment.
Some more minor issues plus several suggestions for further refactoring. I looked at the code in IDE but still didn't find any serious issues. This is as good as you get from me at the moment. To be more useful reviewer, I would need to contribute more regularly to better understand the logic of each component.
| @@ -193,19 +197,20 @@ def aggregate(user_model: object, request: prediction_pb2.SeldonMessageList) -> | |||
| request_json = json_format.MessageToJson(request) | |||
| response_json = user_model.aggregate_rest(request_json) | |||
| return json_to_seldon_message(response_json) | |||
| if hasattr(user_model, "aggregate_grpc"): | |||
| elif hasattr(user_model, "aggregate_grpc"): | |||
There was a problem hiding this comment.
are you sure this kind of checks will still work if you inherit from SeldonComponent? all attributes will be in the child class
There was a problem hiding this comment.
Was trying to get round some type check failures. Solved by allowing Any at certain parts as the user is not required to sub class SeldonComponent at present. Will push an update.
There was a problem hiding this comment.
I liked the idea of having to instantiate SeldonComponent but then - yes - you can't rely on hasattr methods and should use a different mechanism. I reckon you could make it an Interface with no methods, and then branch from it child interfaces with specific set of methods (my guess is that not every method would form a separate interface but you could group some). Users would then inherit several interfaces they need and you would check if their class is an instance of specific interfaces instead of using hasattr. This way you could keep strong typing. It sounds more Java, I know, but with type annotations that's where Python seems to be shifting a bit. :-)
I realise that even if you agree, this a big change, probably requires a bit more thinking, and shouldn't go in this PR. I think using Any is a good solution for now. Not very elegant but works.
There was a problem hiding this comment.
Yes I think that all makes a lot of sense. We should add an issue when this is merged.
…ings which was workaround for type check failures
Rewrite of python wrapper