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

Add language detection to REST API #659

Merged
merged 17 commits into from
Sep 17, 2024

Conversation

UnniKohonen
Copy link
Contributor

@UnniKohonen UnniKohonen commented Dec 29, 2022

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 IETF BCP 47 ISO 639-1 codes. For example:

{
  "languages": ["en", "fi"],
  "text": "A quick brown fox jumped over the lazy dog."
}

The response is a json object with the format:

{
  "results": [
    {"language": "en", "score": 0.85},
    {"language": "fi", "score": 0.1},
    {"language": null, "score": 0.1} 
  ]
}

where the scores range from 0 to 1 and a null value is used for an unknown language.

Implements REST API part of #631.


@juhoinkinen: I edited this for the latest changes (parameter name change candidates -> languages).

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.65%. Comparing base (337ee70) to head (36b479a).
Report is 19 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   99.64%   99.65%           
=======================================
  Files          91       93    +2     
  Lines        6831     6889   +58     
=======================================
+ Hits         6807     6865   +58     
  Misses         24       24           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juhoinkinen
Copy link
Member

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.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start. I gave some comments on individual code lines. In addition to those:

  • black formatting should be applied (see details)
  • there should be a unit test in tests/test_rest.py which exercises the detect_language method

annif/openapi/annif.yaml Outdated Show resolved Hide resolved
annif/openapi/annif.yaml Outdated Show resolved Hide resolved
annif/openapi/annif.yaml Outdated Show resolved Hide resolved
annif/rest.py Outdated Show resolved Hide resolved
@osma
Copy link
Member

osma commented Jan 16, 2023

Thanks for adding tests. A few more things:

  1. There should be tests for special cases, e.g. empty input, no candidate languages, unknown or malformed candidate languages...
  2. What if there are 20 candidate languages? How much memory will it take to handle the query? Will the memory be released afterwards? (I think not, Simplemma keeps models in memory AFAIK)

@UnniKohonen
Copy link
Contributor Author

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.

@osma
Copy link
Member

osma commented Jan 18, 2023

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! 😁

My opinions on these cases:

  1. With no candidates given, I think it would be good to return a 400 Bad Request status code, with a descriptive error message.

  2. 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.

@juhoinkinen
Copy link
Member

juhoinkinen commented Jan 18, 2023

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.

@sonarcloud
Copy link

sonarcloud bot commented Jan 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@juhoinkinen
Copy link
Member

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).

@juhoinkinen
Copy link
Member

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).

Background to the question of test_swagger.py vs test_rest.py: #551 (comment)

@osma osma force-pushed the issue631-rest-api-language-detection branch from 34c2538 to 1cd8003 Compare September 16, 2024 10:53
@osma
Copy link
Member

osma commented Sep 16, 2024

Rebased on PR #724, adapted to use annif.simplemma_util and Connexion 3 support, force-pushed.

@osma osma force-pushed the issue631-rest-api-language-detection branch from 1cd8003 to 7ccbbf0 Compare September 16, 2024 12:09
@osma osma force-pushed the issue631-rest-api-language-detection branch from 7ccbbf0 to 065bfeb Compare September 16, 2024 12:19
@osma osma marked this pull request as ready for review September 16, 2024 12:19
@osma osma force-pushed the issue631-rest-api-language-detection branch from c37ee41 to 36b479a Compare September 16, 2024 13:50
Copy link

sonarcloud bot commented Sep 16, 2024

@juhoinkinen juhoinkinen added this to the 1.2 milestone Sep 17, 2024
Copy link
Member

@juhoinkinen juhoinkinen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with some quick ways, works nicely!

@juhoinkinen juhoinkinen merged commit c42a93f into main Sep 17, 2024
17 checks passed
@juhoinkinen juhoinkinen deleted the issue631-rest-api-language-detection branch September 17, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language detection method in REST API & CLI
3 participants