Allow single token to be annotated by multiple entities in test stories #10773

JEM-Mosig · 2022-01-28T14:30:14Z

This continues the work from here: #10394

Proposed changes:

...

Status (please check what you already did):

added some tests for the functionality
updated the documentation
updated the changelog (please check changelog for instructions)
reformat files using black (please check Readme for instructions)

and removed debug output

…raoulvm/rasa into 2.8.14-test_multi_entities_only

to pass `poetry run flake8 rasa tests --extend-ignore D`

@erohmensing

"D415 First line should end with a period, question mark, or exclamation point" With best regards to @erohmensing ...

to work with overlapping entities.

redoing canceled test at https://github.com/RasaHQ/rasa/runs/4332261715?check_suite_focus=true

though I do not have changed that code block

…raoulvm/rasa into 2.8.14-test_multi_entities_only

JEM-Mosig

A first few changes...

changelog/10394.improvement.md

rasa/shared/nlu/training_data/entities_parser.py

github-actions · 2022-02-03T08:58:10Z

The model regression tests have started. It might take a while, please be patient.
As soon as results are ready you'll see a new comment with the results.

Used configuration can be found in the comment.

raoulvm · 2022-02-03T11:23:29Z

Great work, thanks for your effort!

github-actions · 2022-02-04T06:49:57Z

Commit: 82d3aec, The full report is available as an artifact.

raoulvm · 2022-02-04T09:18:25Z

Hi @JEM-Mosig I had a look at the regression tests... The data for those tests seem to include invalid data, such as

UserWarning: Misaligned entity annotation in message 'set the time zone to g. m. t.' with intent 'datetime_convert'. Make sure the start and end values of entities ([(21, 29, 'g. m. t.')]) in the training data match the token boundaries ([(0, 3, 'set'), (4, 7, 'the'), (8, 12, 'time'), (13, 17, 'zone'), (18, 20, 'to'), (21, 22, 'g'), (24, 25, 'm'), (27, 28, 't')]). Common causes:

entities include trailing whitespaces or punctuation
the tokenizer gives an unexpected result, due to languages such as Chinese that don't use whitespace for word separation
More info at https://rasa.com/docs/rasa/nlu-training-data

I searches the repo for those test strings but didn't see them in rasa. Wanted to help but could not. :-(

JEM-Mosig · 2022-02-04T09:57:47Z

@raoulvm Thanks! Yes, that's mainly why I ran those regression tests. It's so easy to overlook something and this way we have a better chance of catching such problems. I'll look into this today.

@rctatman Can you please review the docs changes of this PR in the meantime? It's not a big change.

JEM-Mosig · 2022-02-04T16:11:50Z

So the warnings come from here, but these are related to other issues that also appear in the released versions. The tests fail due to out of memory problems, however. 🤦‍♂️

rctatman · 2022-02-04T20:37:44Z

docs/docs/testing-your-assistant.mdx

@@ -323,6 +344,20 @@ your trainable entity extractors are trained to recognize.
 Only trainable entity extractors, such as the `DIETClassifier` and `CRFEntityExtractor` are
 evaluated by `rasa test`. Pretrained extractors like the `DucklingHTTPExtractor` are not evaluated.

+If you have multiple entity extractors in your pipeline, or use some custom extractors,
+it is possible that multiple entities are associated with the same token. In this case,


"multiple entities can be associated"?

rctatman

Docs changes LGTM, left on comment on a bit of phrasing that seemed awkward but isn't obligatory to change. Otherwise very clear.

JEM-Mosig · 2022-02-07T08:39:49Z

Ok, looks like there is one last test failing. I'll try to fix that today. Afterwards, I'd like another review from @tayfun. In particular, I had to add two docstrings for classes that are not in the research codebase, eventhough I didn't touch them (the linter complains otherwise).

raoulvm · 2022-02-07T09:04:57Z

I am not sure you'll be able to fix this worker crash by changing the code...
Continuous Integration / Run Tests (test-nlu-featurizers, windows-latest, 3.7) (pull_request)

================================== FAILURES ===================================
[55](https://github.com/RasaHQ/rasa/runs/5069805251?check_suite_focus=true#step:14:55)
_________________ tests/nlu/featurizers/test_lm_featurizer.py _________________
[56](https://github.com/RasaHQ/rasa/runs/5069805251?check_suite_focus=true#step:14:56)
[gw1] win32 -- Python 3.7.9 d:\a\rasa\rasa\.venv\scripts\python.exe
[57](https://github.com/RasaHQ/rasa/runs/5069805251?check_suite_focus=true#step:14:57)
worker 'gw1' crashed while running 'tests/nlu/featurizers/test_lm_featurizer.py::TestShapeValuesTrainAndProcess::test_lm_featurizer_shape_values_process[bert-bert-base-uncased-texts1-expected_shape1-expected_sequence_vec1-expected_cls_vec1]'

And it only crashed with Python 3.7 environment.

JEM-Mosig · 2022-02-07T16:43:38Z

GitHub is broken and we can't open the Files tab for reviews (it yields an Error 500). I reached out to GitHub. But if it's not fixed by tomorrow morning I'll create a separate PR to work around this.

tayfun

You broke Github but 💯

raoulvm · 2022-02-08T07:08:58Z

I'd never expected this to be such a braking change.

JEM-Mosig · 2022-02-08T07:41:47Z

It works again! (I notified Tayfun and Thomas on Slack)

rasa/nlu/test.py

tttthomasssss

Only have a small (and optional) suggestion, otherwise LGTM.

raoulvm and others added 23 commits November 25, 2021 12:09

Allow multiple entities for one token

ba76878

Formatted w/black

a2b5380

and removed debug output

Merge branch '2.8.x' into 2.8.14-test_multi_entities_only

1f923e2

line-length=88

a528aee

Merge branch '2.8.14-test_multi_entities_only' of https://github.com/…

80430f0

…raoulvm/rasa into 2.8.14-test_multi_entities_only

black --line-length 88

8620ca2

to pass `poetry run flake8 rasa tests --extend-ignore D`

add changelog

1b15ef8

Fix docstring liniting in test.py:1091

4603677

"D415 First line should end with a period, question mark, or exclamation point" With best regards to @erohmensing ...

Change test to allow None extractor

cde992a

to work with overlapping entities.

Spelling in changelog

29262b4

Revert accidential revert.

829195b

Some docstring text to force

60dcf2e

redoing canceled test at https://github.com/RasaHQ/rasa/runs/4332261715?check_suite_focus=true

Merge branch '2.8.x' into 2.8.14-test_multi_entities_only

15d909e

Merge branch '2.8.x' into 2.8.14-test_multi_entities_only

0b5aac3

init version of multiple entity writer

ca3bcb6

satisfy make lint

558de89

though I do not have changed that code block

reduce complexity for mypy test

b0c50d6

assert starts AND ends are equal

1287d1c

Merge branch '2.8.x' into 2.8.14-test_multi_entities_only

0ad61f3

activate the change :-)

d296d49

Merge branch '2.8.14-test_multi_entities_only' of https://github.com/…

6b36dcf

…raoulvm/rasa into 2.8.14-test_multi_entities_only

make generate_message more readable

1142e08

Merge branch '2.8.x' into 2.8.14-test_multi_entities_only

f27af50

JEM-Mosig commented Jan 28, 2022

View reviewed changes

changelog/10394.improvement.md Outdated Show resolved Hide resolved

changelog/10394.improvement.md Outdated Show resolved Hide resolved

rasa/shared/nlu/training_data/entities_parser.py Outdated Show resolved Hide resolved

Johannes E. M. Mosig and others added 6 commits January 28, 2022 15:34

Update changelog/10394.improvement.md

e9b1baf

Update changelog/10394.improvement.md

a216b3b

Update rasa/shared/nlu/training_data/entities_parser.py

cb8750b

Add tests and shorten code

227fb4f

Merge branch '2.8.x' into 2.8.18-multi-entities-johannes

d2668e6

Remove timeout marks

8fb021c

Add docstrings

1b13b4f

JEM-Mosig removed the status:model-regression-tests label Feb 4, 2022

rctatman reviewed Feb 4, 2022

View reviewed changes

rctatman approved these changes Feb 4, 2022

View reviewed changes

JEM-Mosig requested a review from tayfun February 7, 2022 09:13

Fix docs

6ca138a

JEM-Mosig marked this pull request as ready for review February 7, 2022 09:19

JEM-Mosig requested review from a team and tttthomasssss and removed request for a team February 7, 2022 09:19

tayfun approved these changes Feb 7, 2022

View reviewed changes

tttthomasssss reviewed Feb 8, 2022

View reviewed changes

rasa/nlu/test.py Outdated Show resolved Hide resolved

tttthomasssss approved these changes Feb 8, 2022

View reviewed changes

Use isdisjoint

39a0383

JEM-Mosig enabled auto-merge (squash) February 8, 2022 09:55

JEM-Mosig merged commit b506c70 into 2.8.x Feb 8, 2022

JEM-Mosig deleted the 2.8.18-multi-entities-johannes branch February 8, 2022 10:24

raoulvm mentioned this pull request Feb 8, 2022

Allow multiple entities for one token #10394

Closed

4 tasks

JEM-Mosig mentioned this pull request Feb 10, 2022

Port PR 10773 #10859

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow single token to be annotated by multiple entities in test stories #10773

Allow single token to be annotated by multiple entities in test stories #10773

JEM-Mosig commented Jan 28, 2022 •

edited

Loading

JEM-Mosig left a comment

github-actions bot commented Feb 3, 2022

raoulvm commented Feb 3, 2022

github-actions bot commented Feb 4, 2022

raoulvm commented Feb 4, 2022

JEM-Mosig commented Feb 4, 2022

JEM-Mosig commented Feb 4, 2022 •

edited

Loading

rctatman Feb 4, 2022

rctatman left a comment

JEM-Mosig commented Feb 7, 2022

raoulvm commented Feb 7, 2022

JEM-Mosig commented Feb 7, 2022 •

edited

Loading

tayfun left a comment

raoulvm commented Feb 8, 2022 •

edited

Loading

JEM-Mosig commented Feb 8, 2022 •

edited

Loading

tttthomasssss left a comment

Allow single token to be annotated by multiple entities in test stories #10773

Allow single token to be annotated by multiple entities in test stories #10773

Conversation

JEM-Mosig commented Jan 28, 2022 • edited Loading

JEM-Mosig left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2022

raoulvm commented Feb 3, 2022

github-actions bot commented Feb 4, 2022

raoulvm commented Feb 4, 2022

JEM-Mosig commented Feb 4, 2022

JEM-Mosig commented Feb 4, 2022 • edited Loading

rctatman Feb 4, 2022

Choose a reason for hiding this comment

rctatman left a comment

Choose a reason for hiding this comment

JEM-Mosig commented Feb 7, 2022

raoulvm commented Feb 7, 2022

JEM-Mosig commented Feb 7, 2022 • edited Loading

tayfun left a comment

Choose a reason for hiding this comment

raoulvm commented Feb 8, 2022 • edited Loading

JEM-Mosig commented Feb 8, 2022 • edited Loading

tttthomasssss left a comment

Choose a reason for hiding this comment

JEM-Mosig commented Jan 28, 2022 •

edited

Loading

JEM-Mosig commented Feb 4, 2022 •

edited

Loading

JEM-Mosig commented Feb 7, 2022 •

edited

Loading

raoulvm commented Feb 8, 2022 •

edited

Loading

JEM-Mosig commented Feb 8, 2022 •

edited

Loading