Open UnniKohonen opened 1 year ago
Base: 99.55% // Head: 99.53% // Decreases project coverage by -0.02%
:warning:
Coverage data is based on head (
34c2538
) compared to base (45be34d
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I see this is only a draft at the moment, but I took a glance and I think it would be better that the end-point name had hyphen instead of underscore (/detect_language
- > /detect-language
), it seems to be the preferred convention.
Thanks for adding tests. A few more things:
Right now, when making a request with no candidates or unknown candidates, the endpoint returns an empty list and when making a request with no text, it returns { "language": null, "score": 1 }
. Does this make sense? Or should it always return the unknown language with score 1 when the input is incorrect?
I also tested making a request with all 48 possible language candidates. I had about 4 GB of free memory which was used amost completely after making the request. The memory isn't released automatically afterwards but it is freed if the endpoint is accessed again (simplemma is run again) or annif is restarted. Making other requests also slows down a lot after runnig simplemma with all candidates.
Right now, when making a request with no candidates or unknown candidates, the endpoint returns an empty list
The good news is that it's not crashing! :grin:
My opinions on these cases:
With no candidates given, I think it would be good to return a 400 Bad Request status code, with a descriptive error message.
With unknown candidates (language codes that are unrecognized/unsupported by simplemma), I think a 400 Bad Request with a descriptive error message would also be appropriate.
There should be unit tests to check that these are indeed the results.
and when making a request with no text, it returns { "language": null, "score": 1 }. Does this make sense? Or should it always return the unknown language with score 1 when the input is incorrect?
I think this is OK, but there should also be a unit test for this special case.
I also tested making a request with all 48 possible language candidates. I had about 4 GB of free memory which was used amost completely after making the request. The memory isn't released automatically afterwards but it is freed if the endpoint is accessed again (simplemma is run again) or annif is restarted. Making other requests also slows down a lot after runnig simplemma with all candidates.
Great, thanks for testing! This is mostly what I suspected, although it's a surprise that accessing the endpoint again will free the memory. (Maybe this has to do with Flask running in development mode?)
This has some potential for DoS situations (intended or not), but I guess it's hard for us to avoid that given how Simplemma works. We could, however, limit the number of candidate languages per request to, say, at most 5. What do others think? @juhoinkinen ?
We could also try to work with the Simplemma maintainer if we want to change the way Simplemma allocates and releases models. For example, it could be possible to ask Simplemma to release the memory immediately or after a set period like 60 seconds after use.
I also tested making a request with all 48 possible language candidates. I had about 4 GB of free memory which was used amost completely after making the request. The memory isn't released automatically afterwards but it is freed if the endpoint is accessed again (simplemma is run again) or annif is restarted. Making other requests also slows down a lot after runnig simplemma with all candidates.
Great, thanks for testing! This is mostly what I suspected, although it's a surprise that accessing the endpoint again will free the memory. (Maybe this has to do with Flask running in development mode?)
This has some potential for DoS situations (intended or not), but I guess it's hard for us to avoid that given how Simplemma works. We could, however, limit the number of candidate languages per request to, say, at most 5. What do others think? @juhoinkinen ?
Limiting the number of candidate languages seems reasonable. If there is no simple way to make the limit configurable, 5 could be a good number for that.
We could also try to work with the Simplemma maintainer if we want to change the way Simplemma allocates and releases models. For example, it could be possible to ask Simplemma to release the memory immediately or after a set period like 60 seconds after use.
I noticed there is an issue in Simplemma repository about loading models to memory, which was opened just yesterday.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells
No Coverage information
0.0% Duplication
Just started to think, if some some testing could be performed also in tests/test_swagger.py. I don't remember just what more functionality does the tests in test_swagger.py
cover than those in test_rest.py
(if any).
Just started to think, if some some testing could be performed also in tests/test_swagger.py. I don't remember just what more functionality does the tests in
test_swagger.py
cover than those intest_rest.py
(if any).
Background to the question of test_swagger.py
vs test_rest.py
: https://github.com/NatLibFi/Annif/pull/551#issuecomment-1011063687
This PR adds the ability to detect the language of a text to the REST API. The language detection uses the simplemma python library.
A POST method is added to the end-point
/detect-language
. It expects the request body to include a json object with the text whose language is to be detected and a list of candidate languages as their ISO 639-1 codes. For example:The response is a json object with the format:
where the scores range from 0 to 1 and a
null
value is used for an unknown language.Implements #631