fix(analyzer): keep Stanza multi-word tokens as surface tokens to preserve text and entity offsets - #2253
fix(analyzer): keep Stanza multi-word tokens as surface tokens to preserve text and entity offsets#2253ManoharPaturi wants to merge 5 commits into
Conversation
…serve text and entity offsets Stanza's mwt processor expands German contractions (im/am/zum/zur/beim/ vom/ins/ans) into multiple words. StanzaTokenizer.__get_tokens_with_heads flattened the expanded words, which broke alignment with the original text: _convert_doc then replaced the doc text with space-separated expanded tokens, silently dropped every NER entity (offsets no longer mapped to tokens), and, on indented text, let tokens_indices end before pattern matches near the end of the text, raising 'Did not find word ...' in LemmaContextAwareEnhancer (HTTP 500). Keep a multi-word token as a single surface token (MultiWordTokenSurface: text/lemma are the surface form, other annotations come from the first expanded word) and remap 1-based word heads to the collapsed token list so dependency heads around the collapsed token stay correct. Fixes data-privacy-stack#2249
|
Hi @ManoharPaturi 🚀 I pulled branch Linting and formatting pass cleanly via Great fix! +1 for maintainers to approve workflow execution and merge. |
|
thanks a lot for pulling the branch and testing it yourself, glad it holds up across the contractions. appreciate the detailed writeup. |
| @@ -0,0 +1,137 @@ | |||
| # Fix: StanzaNlpEngine German multi-word tokens replace the doc text and drop entities (#2249) | |||
| head = ( | ||
| offset | ||
| + word_index_to_token_index.get(token.head - 1, token_index) | ||
| - len(tokens) | ||
| ) |
|
@omri374 removed, thanks. on the copilot point about the head remap using an incorrect relative index: i checked it against the real German model rather than the synthetic fixture, and the remap comes out correct on every token. for 'Wir treffen uns im Büro mit Thomas Bergmann.' stanza gives words (in, dem) as ids 4,5 both governed by id 6 (Büro), which collapses to token 3 governed by token 4:
the offset + word_index_to_token_index term is the governor's absolute doc index and len(tokens) is the current token's absolute index at that point, so the difference is the relative head. happy to add this sentence as a regression test if useful. |
|
Hey @ManoharPaturi! 🚀 Glad the German model trace held up and disproved the Copilot bot finding. Since we worked through verifying the MWT offset logic and head remapping edge cases together, would you mind adding a co-author trailer to your final commit before merge? You can just add this to the end of the commit message: Co-authored-by: Arjun Sanjay Pakhan arjunpakhan@gmail.com Appreciate it, and great job getting this fix put together! |
| self.sentences = sentences | ||
|
|
||
|
|
||
| def test_get_tokens_with_heads_collapses_mwt_and_remaps_heads(): |
There was a problem hiding this comment.
Please add @pytest.mark.skip_engine("stanza_de"). We don't want to download a german stanza model on every unit test run
| assert heads == [1, 0, 1, -2, 1, 0] | ||
|
|
||
|
|
||
| @pytest.mark.skip_engine("stanza_en") |
There was a problem hiding this comment.
should this be @pytest.mark.skip_engine("stanza_de")?
| assert doc.text[token.idx : token.idx + len(token.text)] == token.text | ||
|
|
||
|
|
||
| @pytest.mark.skip_engine("stanza_en") |
There was a problem hiding this comment.
| @pytest.mark.skip_engine("stanza_en") | |
| @pytest.mark.skip_engine("stanza_de") |
| assert doc.ents[0].end_char == expected_start + len("Thomas Bergmann") | ||
|
|
||
|
|
||
| @pytest.mark.skip_engine("stanza_en") |
There was a problem hiding this comment.
| @pytest.mark.skip_engine("stanza_en") | |
| @pytest.mark.skip_engine("stanza_de") |
omri374
left a comment
There was a problem hiding this comment.
Thanks! The only thing missing is proper handling of downloading Stanza models during unit tests. You'd might need to update the conftest here:
https://github.com/ManoharPaturi/presidio/blob/b71a4b4aeaa889a0f51eb2c7d0612fc1702b1619/presidio-analyzer/tests/conftest.py#L31
To add stanza_de as a preloaded NLP Engine
Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com> Co-authored-by: Arjun Sanjay Pakhan <arjunpakhan@gmail.com>
|
done, trailer added to the final commit. thanks for the thorough verification pass, that kind of pull-the-branch testing is exactly what the fix needed. |
3c79b36 to
6bdb5c6
Compare
…erman tests Add stanza_de to nlp_engines fixture and provide stanza_de_nlp_engine. Update German Stanza integration tests to gate on stanza_de engine and use preloaded pipelines from nlp_engines when available. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
|
@omri374 Updated! Added |
|
@omri374 following up on this one. the notes file is removed and i added the co-author trailer arjunpakhan asked for. his verification pass plus your main merge are on the thread. anything else needed before this can be merged? |
Fixes #2249.
StanzaTokenizer.__get_tokens_with_headsflattened stanza multi-word-token expansions (im->in+dem), so token texts no longer matched the source text._convert_docthen rebuilt the doc text from those tokens, which corrupted the document text (see the issue repro), made Stanza NER spans miss every token boundary so entities were silently dropped, and could raiseValueErrorfrom the lemma enhancer on indented text.Multi-word tokens now stay a single surface token (a small wrapper carrying surface text/lemma plus the first expanded word's annotations), with a word-index to token-index map that remaps dependency heads across the collapsed token. Single-word tokens keep the previous code path byte-for-byte.
Verified with the real German model:
doc.textis preserved,PERandIP_ADDRESSentities are detected where pristine drops them. 11 new tests (pure-unit head remap + German model tests over all 8 contractions) fail on main and pass here; existing stanza suites pass unchanged (12 + 13 + 16), ruff clean vs main baseline.