update user messages in apply_to of define events

[Ghostvv]

Proposed changes:

• use apply_to methods instead of tracker.applied_events

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)

[tabergma]

🚀

[Ghostvv]

@tabergma I was wrong, sorry. Things are a bit more complicated 🙈: The batch dimension of entity prediction is not of batch size, but of number of last (if max history featurizer else all) text inputs in the batch

[wochinge]

Nearly there but not entirely.

This still fails because self.latest_message is just a pointer to the event.

def test_tobi():
    tracker = DialogueStateTracker.from_events(
        "Vova",
        evts=[
            ActionExecuted(ACTION_LISTEN_NAME),
            UserUttered("hi", intent={"name": "greet"}),
            DefinePrevUserUtteredFeaturization(True),
            DefinePrevUserUtteredEntities(
                entities=[{"entity": "entity1", "value": "value1"}]
            ),
        ],
    )

    user = list(tracker.events)[1]
    assert isinstance(user, UserUttered)

    # Fails
    assert not user.entities

[Ghostvv]

it should fail

[Ghostvv]

we want user uttered to have entities

[wochinge]

but this means that when we store the tracker, we store the updated version of UserUttered which would change history.

[Ghostvv]

that's fine

[wochinge]

There is no need to rely on a fragile thing such as the time of persistence.

We should only persist UserUttered event after policy prediction happened and all the policy events are applied

There is no need for that as we have the two additional / new events. We will just replay events and come back to the current state. When we discussed the timing of persistence, it was when we considered if we need the additional events after all.

[Ghostvv]

yeah, but we use them differently

[Ghostvv]

It means that the TrackerStore is no longer a 1:1 presentation of what happened.

which is good, since we effectively made UserUttered a dynamic event

[wochinge]

This is good for the ML models, but it's not good for the system itself. Breaking the immutability property is opening the box of the pandora in my opinion.

[Ghostvv]

I saw the other comment, I see your point

[Ghostvv]

@wochinge since we established that the problem exists regardless of this change, I'd like to merge this PR, because it contains other fixes

[wochinge]

I left another comment on the ActionExecuted class which should be considered.
We can continue the discussion for the mutating the UserUttered event on the other branch.

[Ghostvv]

@wochinge do you have an idea how the changes in this PR break the tests that fail. I don't understand why they're dependent

[Ghostvv]

@wochinge for some reason this change breaks everything. If I don't include Define... events in applied_events sometimes they're not applied. I'll remove it from here. Anyhow, this should be redesigned

[wochinge]

Mhm, then we should investigate why this breaks everything. That can only mean that events are not applied properly to the tracker.

[wochinge]

Maybe the problem is that your apply_to implementations manipulate last_user_message and last_action instead of the events tracker.events 🤔 They should refer to the same things, but maybe some stuff is actually deepcopying it.

[wochinge]

ah, now I understand it! Yes, you definitely have to still add the event as applied events.

[wochinge]

If you don't add it, then the events are not applied in the new tracker (as it doesn't get them) and everything will break

[wochinge]

but the new tracker should already get new updated UserUttered

There is no need to. The new tracker will call apply_to on each new event

[Ghostvv]

but then we do it million of times, doesn't feel efficient for me

[wochinge]

We actually need to do this for all of the events (e.g. restarted needs to reset a lot of state). I think the problem is rather in the featurization

[Ghostvv]

what do you mean? in core featurizers? Theoretically they should always receive UserUttered events where their featurization is decided

[wochinge]

I'm not sure about this. Anyway, let's close this discussion and have it in comment of the large PR