Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next steps End-to-end: Implementation Proposal + Implementation #7039

Closed
wochinge opened this issue Oct 16, 2020 · 4 comments · Fixed by #7136
Closed

Next steps End-to-end: Implementation Proposal + Implementation #7039

wochinge opened this issue Oct 16, 2020 · 4 comments · Fixed by #7136
Assignees
Labels
type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Milestone

Comments

@wochinge
Copy link
Contributor

Short scope document

Policies have to be able to return events to store information about the used featurization in the tracker.

@wochinge wochinge added type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR priority:high labels Oct 16, 2020
@wochinge
Copy link
Contributor Author

I think it would be cool to use a context manager to save the tracker. Currently we save the tracker quite often in between which is 1) Complex from a code-perspective and 2) Slow.

I'd imagine something like

with tracker_store.get_tracker(sender_id) as tracker:
   # ... do things

This would mean that a tracker would only be saved after at the end of the interaction (aka we're waiting for the next user message).

@wochinge wochinge added this to the e2e milestone Oct 20, 2020
@wochinge
Copy link
Contributor Author

@Ghostvv I thought about it some more:

  • I'd implement "policies returning events" against master. This keeps the diff for end-to-end smaller and you can just use it in your e2e implementation then
  • We need a deprecation warning for policies not returning the new items
  • I think DefinePrevUserUtteredFeaturization could use apply_to to update the UserUttered event instead of applied_events. This way the whole logic would be nicely encapsulated within the event and not spread out too much.
  • I think a separate event is indeed better than manipulating the UserUttered event directly. This has two advantages for me
    • more in line with our philosophy of unmodifiable events and that the tracker events give a precise history of what happened
    • allows the policies to manipulate the tracker by only using events. Directly modifying the event would require some less abstracted operations which are less generalizable / reusable.

My idea from the comment above would be a nice refactoring, but it's not required for these changes

@Ghostvv
Copy link
Contributor

Ghostvv commented Oct 21, 2020

I'd implement "policies returning events" against master.

ok, the only problem, that you couldn't properly test it

I think DefinePrevUserUtteredFeaturization could use apply_to to update the UserUttered event instead of applied_events.

as we discussed I prefer this one as well, but it'll work only if trackers are stored at the right moment. I don't think it'll work reliably without your idea above

more in line with our philosophy of unmodifiable events and that the tracker events give a precise history of what happened

but if you do it with DefinePrevUserUtteredFeaturization.apply_to you will modify UserUttered event

@wochinge
Copy link
Contributor Author

ok, the only problem, that you couldn't properly test it

I mean your approach already works, right? That would simply be the same changes just a bit more polished. And we can write tests for it and so on. Worst case is that you can cherry pick the changes and we quickly try it out in e2e?

as we discussed I prefer this one as well, but it'll work only if trackers are stored at the right moment. I don't think it'll work reliably without your idea above

When we do DialogueStateTracker.from_events we call tracker.update for every event which in turn calls apply_to for every event (same applies to TrackerWithCachedStates). So that would be applied independent of when events are persisted.

but if you do it with DefinePrevUserUtteredFeaturization.apply_to you will modify UserUttered event

But not in the database. applied_events in your branch is currently doing the same modification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants