fix ted training e2e entities when none are given

[Ghostvv]

Proposed changes:

• fix ted training e2e entities when none are given in the stories but there are entities in the domain

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)

[Ghostvv]

@joejuzl could you please help me with the test? Testing that TEDPolicy turns off entity_recognition parameter requires trained nlu model and I'm not sure where to write such a test

[joejuzl]

@joejuzl could you please help me with the test? Testing that TEDPolicy turns off entity_recognition parameter requires trained nlu model and I'm not sure where to write such a test

There is a fixture called e2e_bot which has a trained NLU model.

[Ghostvv]

There is a fixture called e2e_bot which has a trained NLU model.

how can I use it? 🙈

[Ghostvv]

There is a fixture called e2e_bot which has a trained NLU model.

oh, I need e2e_bot without e2e entities

[joejuzl]

What is it you exactly want to test? A TED with entity extraction turned on but no entities in the training data?

[Ghostvv]

A TED with entity extraction turned on but no entities in the training data?

yes the problem is that entity extraction is turned on by default, we need to turn it off if there are no e2e entities, currently it turns off only if there is no e2e stories or no entities defined in the domain file. This fix turns it off if there is e2e user text in the stories but no e2e entities. I'd like to add a test for this to make sure we don't accidentally remove this fix in the future

[Ghostvv]

@joejuzl any ideas how to test it?

[joejuzl]

@joejuzl any ideas how to test it?

hmm are there no tests that train TED on e2e data?

[Ghostvv]

hmm are there no tests that train TED on e2e data?

that's what I was asking 🙈

[joejuzl]

that's what I was asking 🙈

Ok well, there are tests in test_model_training.py that test e2e from the scope of loading files and fingerprinting, but they mock the actual testing calls.

Then there is test_e2ebot which is a fixture that trains a simple e2e bot that I used for testing rasa test with e2e entities.

However I would have assumed when the actual changes were made to TED to make it work with e2e that tests would have been created i.e. in test_ted_policy.py to test the actual training and prediction for e2e.
But I'm assuming that's not the case given your question.
So I guess we should create some!

[Ghostvv]

test_ted_policy.py doesn't have access to trained nlu model

[joejuzl]

test_ted_policy.py doesn't have access to trained nlu model

Could you not use a fixture like default_agent, get the interpreter from it, and pass it into TED in the tests?

[Ghostvv]

Could you not use a fixture like default_agent, get the interpreter from it, and pass it into TED in the tests?

I'll try. how can I use the fixture? I don't know what fixture is 🙈

[joejuzl]

I'll try. how can I use the fixture? I don't know what fixture is 🙈

Surely you've used fixtures before? A fixture is any method that has the decorator @pytest.fixture and if it (or its module) has been imported it will be available in any test by having an argument to the test method with the same name as the fixture method.
https://docs.pytest.org/en/stable/fixture.html#what-fixtures-are

Search for default_agent in the test files of the code base and you'll see how it's used.
Let me know if you wanna call to discuss using it in the TED tests, or alternative approaches.

[Ghostvv]

@joejuzl I just took a look and I think implementing e2e ted tests within TestTEDPolicy would require significant refactor of PolicyTestCollection, because whether ted is e2e is controlled by the training data it is trained on. I think it is out of scope of this PR. I propose merge this fix as is, and create a separate issue for e2e ted tests. What do you think?

[joejuzl]

@joejuzl I just took a look and I think implementing e2e ted tests within TestTEDPolicy would require significant refactor of PolicyTestCollection, because whether ted is e2e is controlled by the training data it is trained on. I think it is out of scope of this PR. I propose merge this fix as is, and create a separate issue for e2e ted tests. What do you think?

Yup agreed - adding e2e tests for ted is out of scope for this PR.
However we should make sure we do it soon, as it's a lot of untested functionality...

[Ghostvv]

created the issue for e2e TED tests: #8273