jgontrum / spacy-api-docker

spaCy REST API, wrapped in a Docker container.
https://hub.docker.com/r/jgontrum/spacyapi/
MIT License
265 stars 99 forks source link

New /sent_deps/ endpoint, fix unit tests, add some optional tokenizer config #33

Closed matthewarmand closed 4 years ago

matthewarmand commented 4 years ago
jgontrum commented 4 years ago

Looks amazing! I’ll give it a try tomorrow

matthewarmand commented 4 years ago

Sounds good @jgontrum, let me know if you'd like any extra context on anything, or if anything doesn't make sense or should be different.

jgontrum commented 4 years ago

Looks good so far! One thing I've noticed is that the server returns a 500 if the model in the request is not found. Could you please catch it and return a more informative message as we do with the /dep endpoint?:

{
  "title": "Dependency parsing failed",
  "description": "[E050] Can't find model 'some_unknown_model'. It doesn't seem to be a shortcut link, a Python package or a valid path to a data directory."
}
matthewarmand commented 4 years ago

Looks good so far! One thing I've noticed is that the server returns a 500 if the model in the request is not found. Could you please catch it and return a more informative message as we do with the /dep endpoint?:

Good catch @jgontrum, yes I can definitely make that change. I notice that a couple of the other endpoints are missing that error handling as well; while I'm in here would you like me to add that to /sents and /ent?

Additionally, /models and /version are just returning a 500; if you'd like I can expand that to something a bit more informative, such as:

except Exception as e:
    raise falcon.HTTPInternalServerError(
        'Error occurred retrieving models',
        '{}'.format(e))
matthewarmand commented 4 years ago

@jgontrum check out 4c64c74 for code addressing your feedback.

I added the handling to the new /sents_dep endpoint as you requested, and also added similar handling to /ent and /sents, which were also missing that. I also wrote a parametrized unit test case to cover that behavior for all those endpoints.

I also went ahead and added better 500 handling to /version and /models so a message about what went wrong will be returned.

jgontrum commented 4 years ago

Looks good to me :) Thank you for work!