Add model_id to events in the processor.
#9917
Conversation
| ] | ||
| assert len(tracker.events) == len(expected_events) | ||
| for e1, e2 in zip(tracker.events, expected_events): | ||
| assert e1 == e1 |
There was a problem hiding this comment.
That's how all the tests passed 😁
tests/core/test_processor.py
Outdated
| @@ -579,37 +580,53 @@ async def test_update_tracker_session( | |||
| tracker = default_processor.tracker_store.retrieve(sender_id) | |||
|
|
|||
| assert list(tracker.events) == [ | |||
| # Created by `tracker_store.get_or_create_tracker` so has no model_id | |||
There was a problem hiding this comment.
When this happens within the processor the it uses append_action_listen=False so we don't get this behaviour.
joejuzl
requested review from
aeshky and
wochinge
and removed request for
a team and
aeshky
changelog/8914.improvement.md
Outdated
| @@ -0,0 +1 @@ | |||
| The model identifier, `model_id`, is now added to the `Event` metadata. | |||
There was a problem hiding this comment.
How phrasing this more user facing, e.g. "Every conversation event now includes the ID of the model which predicted it in its metadata."?
| ] | ||
| assert len(tracker.events) == len(expected_events) | ||
| for e1, e2 in zip(tracker.events, expected_events): | ||
| assert e1 == e1 |
There was a problem hiding this comment.
That's how all the tests passed 😁
rasa/core/processor.py
Outdated
| @@ -910,6 +911,11 @@ def _has_session_expired(self, tracker: DialogueStateTracker) -> bool: | |||
| return has_expired | |||
|
|
|||
| def _save_tracker(self, tracker: DialogueStateTracker) -> None: | |||
There was a problem hiding this comment.
I'm a bit worried what happens e.g. through interactive learning when we do single predictions and then use the API to append the events to the tracker. Could we add a variable model_id on the DialogueStateTracker which is automatically applied for all tracker.update things? We could set this variable in processor.get_tracker. This would make sure that all events going through the API / SDK etc. have this ID and not just the ones persisted by us.
There was a problem hiding this comment.
And yes, none of these options are pretty 😬
There was a problem hiding this comment.
I think in an ideal world the tracker stores would only return a list of events. The processor would than create a DialogueStateTracker from this using model specific data like domain or model_id. Tracker stores would only persist list of events instead of entire trackers.
There was a problem hiding this comment.
But if events are being created outside of processor e.g. the user is picking something in interactive, should it have the model_id? It seems wrong to me to add it to events that are passed to rasa via the server...
Also if we add the id to the tracker, and then the model changes, but we use the old tracker - the model_id will be wrong.
There was a problem hiding this comment.
If they create the events in interactive then they will still go through the processor. Any outgoing events will then have the model ID of the model which did this single prediction. If they then add these events to Rasa via API, then the events should already have the model ID (unless they dropped in between, but I'd say that's their fault then).
I wouldn't not persist the model_id on the DialogueStateTracker. I'd only add that temporarily during inference (just in the processor). I'd only persist the model_id which is part of the event metadata (there is basically a distinction between a temporary tracker model ID and an event model ID).
…saHQ/rasa into Embed_model_identifier_in_events_8914
| @@ -35,6 +35,7 @@ | |||
| from rasa.shared.core.domain import Domain | |||
| from rasa.shared.constants import INTENT_MESSAGE_PREFIX | |||
| from rasa.utils.endpoints import EndpointConfig | |||
| from tests.conftest import with_model_ids | |||
There was a problem hiding this comment.
optional: nit but ideally just import the module and use that to refer to the function (python code conventions)
| assert deserialized_events[3] == event | ||
| assert deserialized_events[:3] == with_model_ids(session_start_sequence, model_id) | ||
|
|
||
| assert deserialized_events[3] == with_model_id(event, model_id) |
There was a problem hiding this comment.
What if your posted event already has a model_id?
There was a problem hiding this comment.
in my opinion that one shouldn't be overriden
There was a problem hiding this comment.
I feel like this could lead to some confusion though. And it's clearer to just say that all events added to the tracker will have the model id of the currently loaded model.
There was a problem hiding this comment.
mhm, I somewhat worried that it messes up some reporting. E.g. you post an old tracker from an old model to the API and then want to run predictions on it. In that case I'd expect the "old" events to have the old model ID and any newly predicted events to have the new model ID.
And it's clearer to just say that all events added to the tracker will have the model id of the currently loaded model.
How about "ID of the model which was used during the time of the event creation"?
Proposed changes:
Closes #8914
Status (please check what you already did):
black(please check Readme for instructions)