Skip to content
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

Defining the model serving class with full name doesn't currently work #533

Closed
hmonteiro opened this issue Apr 29, 2019 · 7 comments
Closed
Milestone

Comments

@hmonteiro
Copy link

In line 194 of microservice.py you are trying to load a module from args.interface_name. The problems is that at this point, interface name is the full class name (e.g. package.module.Class) so import_lib.import module fails.

This line has to be changed from interface_file = importlib.import_module(args.interface_name) to interface_file = importlib.import_module(parts[0])

@hmonteiro
Copy link
Author

By the way, I'm really happy with this change, since (after this bug is fixed) I can throw away my hack of setting up a dummy serving class that just forwards the requests to the proper one.

@ukclivecox
Copy link
Contributor

Thanks. Any chance you can do a PR and check why these tests are not adequate:

def test_model_template_app_rest_submodule(tracing):
with start_microservice(join(dirname(__file__), "model-template-app2"),tracing=tracing):
data = '{"data":{"names":["a","b"],"ndarray":[[1.0,2.0]]}}'
response = requests.get(
"http://127.0.0.1:5000/predict", params="json=%s" % data)
response.raise_for_status()
assert response.json() == {
'data': {'names': ['t:0', 't:1'], 'ndarray': [[1.0, 2.0]]}, 'meta': {}}
data = ('{"request":{"data":{"names":["a","b"],"ndarray":[[1.0,2.0]]}},'
'"response":{"meta":{"routing":{"router":0}},"data":{"names":["a","b"],'
'"ndarray":[[1.0,2.0]]}},"reward":1}')
response = requests.get(
"http://127.0.0.1:5000/send-feedback", params="json=%s" % data)
response.raise_for_status()
assert response.json() == {'data': {'ndarray': []}, 'meta': {}}

@hmonteiro
Copy link
Author

I'll try to have a look. If I find the unit test problem I'll do a PR

@hmonteiro
Copy link
Author

hmonteiro commented May 1, 2019

Found the problem, you are still relying on the fact that the filename is the same as the class name. In python the filename should be snake case while class names are camel case, so this won't usually be the case. I'll fix the unit test and the code and will provide a PR.

@ukclivecox
Copy link
Contributor

Thanks. This is great.

@hmonteiro
Copy link
Author

The fix is in the PR, let me know if there is anything else you need.

@ukclivecox
Copy link
Contributor

Closed as merged.

agrski added a commit that referenced this issue Dec 2, 2022
…ommand (#533)

* Use positional args for server name in CLI status subcommand

Requiring a name passed by flag was inconsistent with the other commands requiring resource names.

* Remove now-unused constant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants