Add API to extract terms from provided text - #4516
Conversation
|
(solving merge conflicts) |
|
|
||
|
|
||
| @pytest.fixture | ||
| def terminology_extraction_setup(): |
There was a problem hiding this comment.
Extraction means candidate discovery in terminology lingo, whereas this API matches against an existing glossary.
|
|
||
| def get_translation_text(self, obj): | ||
| if obj.do_not_translate: | ||
| return obj.text |
There was a problem hiding this comment.
This also changes the existing endpoint /api/v2/search/terminology/, but I guess it's the right behaviour and possibly also not impacting the output.
There was a problem hiding this comment.
Good point, forgot to call it out in the PR.
There was a problem hiding this comment.
Note: it doesn't impact search/terminology, because that doesn't return do-no-translate terms (they don't have an entity when created, so no translations).
return queryset.filter(translations__locale__code=value)
I wonder if that behavior should be changed, but in case it's probably a follow-up?
| | Parameter | Description | | ||
| | --------- | --------------------------------------------------------- | | ||
| | `locale` | Locale code | | ||
| | `text` | Text to extract terminology from, at most 2048 characters | |
There was a problem hiding this comment.
Technically this is TERMINOLOGY_API_MAX_CHARS characters. But we can also leave it out in the docs.
| if not text.strip(): | ||
| errors["text"] = ["This field is required."] | ||
| elif len(text) > TERMINOLOGY_API_MAX_CHARS: | ||
| errors["text"] = [ | ||
| f"Text exceeds maximum length of {TERMINOLOGY_API_MAX_CHARS} characters." | ||
| ] |
There was a problem hiding this comment.
Nit: These two conditions are independent, so the elif misreports a 5000-character whitespace-only text as {"text": ["This field is required."]} instead of the length error.
| path( | ||
| "terminology/extract-from-text/", | ||
| views.TermExtractFromTextView.as_view(), | ||
| name="term-extract-from-text", |
There was a problem hiding this comment.
Can we settle the path before this ships? "Extract" in terminology parlance means discovering candidate terms from a corpus, whereas this does the inverse — matching text against the existing glossary. Also, -from-text just repeats the text parameter.
| name="term-extract-from-text", | |
| "terminology/matches/", | |
| views.TermMatchListView.as_view(), | |
| name="term-matches", |
There was a problem hiding this comment.
The reason for using from-text was that this would be coupled with from-file. But at this point, the idea is to leave that part alone for now, and probably have a more generic analysis that does more than just terminology.
On that point, I think I'm going to split the original issue to make that clear, and have this PR fix the existing one.
| return qs | ||
|
|
||
|
|
||
| class TermExtractFromTextView(generics.ListAPIView): |
There was a problem hiding this comment.
Shall we add a throttle class here? Each call SELECTs every Term, prefetches their TermTranslations, and runs one re.search per term over up to 2048 chars.
| return "\n".join(text_parts) | ||
|
|
||
|
|
||
| def join_text_fragments(texts: Iterable[str]) -> str: |
There was a problem hiding this comment.
This has no production caller. What did you have in mind for it?
There was a problem hiding this comment.
D'oh. It's a left-over from local iterations where I was testing "upload from file" (this would be needed for complex Fluent strings), making sure that the structure could be build on top.
| ) | ||
| ).order_by("text", "id") | ||
|
|
||
| return terms.for_string(text) |
There was a problem hiding this comment.
The Prefetch above will not prevent the TermTranslation rows to load for every candidate term rather than the matches — a text matching 1 of 1000 terms still pulls 1000 rows and discards them.
I'd suggest something like this:
matched = Term.objects.for_string(text)
return (
Term.objects.filter(pk__in=[t.pk for t in matched])
.prefetch_related(Prefetch(...))
.order_by("text", "id")
)There was a problem hiding this comment.
Using slightly different approach that should use one less query?
prefetch_related_objects(
terms,
Prefetch(
"translations",
queryset=TermTranslation.objects.filter(locale=locale),
to_attr="filtered_translations",
),
)|
|
||
| Extract the terms appearing in a text, with their translation in a given locale. | ||
|
|
||
| Unlike [`/api/v2/search/terminology/`](#json-mode), which looks up terms by name, this |
There was a problem hiding this comment.
| Unlike [`/api/v2/search/terminology/`](#json-mode), which looks up terms by name, this | |
| Unlike [`/api/v2/search/terminology/`](#/search/search_terminology_list), which looks up terms by name, this |
mathjazz
left a comment
There was a problem hiding this comment.
Great work! Just left one minor suggestion inline for the README consistency.
| The endpoint is rate limited per user, or per IP address for anonymous requests, with a | ||
| burst limit of 60 calls per minute and a sustained limit of 600 calls per hour by default | ||
| (configurable via `API_TERMINOLOGY_THROTTLE_BURST` and | ||
| `API_TERMINOLOGY_THROTTLE_SUSTAINED`). Calls over the limit are rejected with `429`. |
There was a problem hiding this comment.
Like for the Upload throttle variables, I'd mention these are independent:
#4503 (comment)
Fixes #4512.
Opening up for review since it's self-contained, while the second part (extract from file) will touch and likely refactor some code being reviewed in #4511.