don't persist changed entities + autofill slots for policy entities

[wochinge]

Proposed changes:

• don't persist UserUttered attribute which were changed by further events
• The tracker we return in the API / send to the action server now contains the combined UserUttered.parse_data in its field latest_message.Combined means that it contains the entities of NLU predictions and entities predicted by the policies. This allows users to access the entities as usual.
• support autofilling slots with entities predicted by policies
• rename DefinePrevUserUtteredEntities to EntitiesAdded

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]

I'm not sure I can understand. So entities will be added every time in applied events? So that users will see "normal" UserUttered in custom actions?

[Ghostvv]

if this is the case, I believe the if on line 728 in events.py should be removed

[wochinge]

I'm not sure I can understand. So entities will be added every time in applied events? So that users will see "normal" UserUttered in custom actions?

Entities are added via apply_to as you implemented it. However, any changes to UserUttered.entities won't affect the serialized version of the event. This gives you the updated entities for the featurization while preserving the original event.
A user would only see the original UserUttered in a custom action (without entities added by policies).

[Ghostvv]

A user would only see the original UserUttered in a custom action (without entities added by policies).}

then it's not very useful, this was the reason for updating UserUttered event, so that these entities are perceived by users exactly the same as extracted by TED

This gives you the updated entities for the featurization while preserving the original event.

we actually don't use e2e entities as features

[wochinge]

we actually don't use e2e entities as features

So only entities / roles / groups from the NLU classification are considered as features?

then it's not very useful, this was the reason for updating UserUttered event, so that these entities are perceived by users exactly the same as extracted by TED

This needs to be done in a different place then. We can't mutate the event. What specific locations are you thinking of when you mean they should be perceived the same as extracted by TED? It should work out of the box for everything except for custom actions.

We could use applied_events instead combined with a copy.deepcopy but then we'd need to make sure we'd call that whenever exposing the tracker to the user. Currently we're e.g. not calling applied_events before sending them to the action server.

[Ghostvv]

So only entities / roles / groups from the NLU classification are considered as features?

yes

[wochinge]

@Ghostvv @joejuzl Updated according to what we discussed. How you can test this:

1. Use the e2e example bot
2. train it
3. Send the message Hello
4. GET <host>:<port>:/conversations/<conversation ID>/tracker will return a tracker where latest_message contains the entities from the TEDPolicy while the actual events are untouched.

[wochinge]

@Ghostvv We should in my opinion also updated this check to work for the new event as well, shouldn't we?

[joejuzl]

@wochinge

GET ::/conversations//tracker will return a tracker where latest_message contains the entities from the TEDPolicy while the actual events are untouched.

So the DefinePrevUserUttered... events now only exist in the tracker within the ensemble?

[wochinge]

So the DefinePrevUserUttered... events now only exist in the tracker within the ensemble?

Nope, it's a regular event as the others as well. We just changed that it does not longer affect the persisted version of UserUttered. In addition we changed the latest_message property of the tracker to make it easier for the users to access the combined entities (e.g. in custom actions)

[joejuzl]

I've tested it an the entities show as expected.

[wochinge]

@joejuzl @Ghostvv Added support for autofilling slots based on entities predicted by polices. Please(re-)review

[Ghostvv]

This solution seems to me very convoluted. Why do we update previous UserUttered with entities? Then we use a lot of different .copy() to mitigate that. Isn't it simpler to store e2e_entities separate on the tracker and add them to latest_message data directly?

[wochinge]

Then we use a lot of different .copy() to mitigate that.

actually just one 😆

Isn't it simpler to store e2e_entities separate on the tracker and add them to latest_message data directly?

The nice thing about having it in entities is that story printing would work as usual. It allows to interact with the event as it'd be one thing.

[Ghostvv]

ok, if you think it is better 🚀

what about renaming? also left one refactoring comment

[Ghostvv]

what about:

if domain:
   if isinstance(event, UserUttered):
   if isinstance(event, DefinePrevUserUtteredEntities):

?

[wochinge]

I think that's style dependent and should be optional to address.

[Ghostvv]

I mean... this is colossal if domain and isinstance(event, (UserUttered, DefinePrevUserUtteredEntities)): and then the solitary else is confusing

[Ghostvv]

it takes more than a second to understand this if for someone else

[Ghostvv]

I don't think it is style dependent, we should make sure that the code is as simple as possible to grasp by looking at it

[wochinge]

I still think it's clear. Happy to hear @joejuzl 's opinion on it.

[Ghostvv]

do we need that? let's just add as_story_string to all the methods

[Ghostvv]

@joejuzl what do you think. this method seems to me redundant but also increases cognitive complexity of events

[joejuzl]

I think it's fine. It means you can easily identify events which won't be in the story rather than having to look at the implementation of the method in each event.

[Ghostvv]

to me it contradicts Occam’s razor principle, but after all it's your part of code

[Ghostvv]

for me personally, it makes it harder to understand, I see the method - I know how it is represented, looking at superclass requires more cognitive function

[joejuzl]

I see arguments for both ways - but in the interest of speed (and since it's already done like this) - I think we should keep it as is.