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

External events and reminders should trigger intents instead of actions #4464

Closed
3 tasks
JEM-Mosig opened this issue Sep 16, 2019 · 16 comments · Fixed by #4964
Closed
3 tasks

External events and reminders should trigger intents instead of actions #4464

JEM-Mosig opened this issue Sep 16, 2019 · 16 comments · Fixed by #4964
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR

Comments

@JEM-Mosig
Copy link

JEM-Mosig commented Sep 16, 2019

Description of Problem:
Reminders and external events always trigger actions, which contradicts the logic that actions should be unambiguously predictable from the dialog history and present intent. For example, in the use case where a reminder is used to have the bot initiate chitchat, this makes stories unlearnable.

Overview of the Solution:
External events and reminders should trigger intents, not actions, since intents are the things that are out of our control and actions should be deterministic. Now that we have a mapping policy, this has no drawbacks.

For some applications and backwards compatibility, triggering actions should still be possible, so both options have to be available.

Examples (if relevant):
If we want to set a reminder to have the bot initiate chitchat, we would now trigger the intent, say, EXT_remind_share_me, which is linked to the action utter_share_me via the MappingPolicy. This results in stories of the form

* EXT_remind_share
  - utter_share_me
* general_confirm
...

which can be learned and are readable.

Definition of Done:

  • Tests are added
  • Feature described in the docs
  • Feature mentioned in the changelog
@JEM-Mosig JEM-Mosig added the type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR label Sep 16, 2019
@JEM-Mosig JEM-Mosig self-assigned this Sep 16, 2019
@wochinge wochinge added the area:rasa-oss 🎡 Anything related to the open source Rasa framework label Oct 24, 2019
@JEM-Mosig
Copy link
Author

It works now in Rasa X, but we have to suppress the user message somehow:
image
Also, to make it work in rasa shell, we need to solve Issue #4340

@JEM-Mosig
Copy link
Author

JEM-Mosig commented Nov 8, 2019

Intent-Reminders work now! See https://github.com/RasaHQ/rasa/tree/johannes-4464.
image
But the code is a bit hacky. For clarity, I think we should introduce a new event type ExternalEvent(Event) that behaves like UserMessage, except that it is not displayed.

Also, to make it work in rasa shell, we need to solve Issue #4340

@tmbo tmbo added the type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. label Nov 8, 2019
@tmbo
Copy link
Member

tmbo commented Nov 8, 2019

I guess you mean the event behaves like UserUttered (because UserMessage is not an event).

Could there be cases where it is better to not have this be directly linked to intents?

Might make sense to introduce this in a major. I do not think we should have two different ways of running reminders. I'd prefer if we remove the old way and maybe try to workaround supporting existing reminders with a new system.

Small remark to the story format: might consider a different indicator (e.g. ! instead of *) infront of the event in the story.

@JEM-Mosig
Copy link
Author

JEM-Mosig commented Nov 8, 2019

Copy of main points from Slack discussion:

I got intent-triggering-reminders to work: #4464 (comment) but I think to make it clean, we should introduce a new type of event ExternalEvent(Event) that behaves like UserMessage, except that it is not displayed. We should also have a WorldMessaged that’s like UserUttered. What do you think?

The reasoning behind this is as follows: From the point of view of Rasa Core, system actions must be deterministic, while the user can do whatever (s)he wants whenever action_listen is active. An external event or reminder is by design not deterministic, i.e. given the dialogue history it is not predictable if a reminder or external event would be triggered next. That also means that whenever you use the current version of reminders, your stories become unlearnable. Therefore, these events should be handled like user input that is not displayed. The intent can then be linked to a system action via the mapping policy (which did not exist when reminders where designed, I think).

For example, let’s say a user goes through Story A and in the end a reminder is triggered. Another user goes through Story A and writes something that cancels the reminder. This results in two stories at the end of which the bot will have to predict either action_remind or action_listen. Rasa Core is not designed to learn this (except if we use @vova’s new probabilistic framework).

I think of it as promoting the story lines * something from indicating a user intent to indicating anything that comes to the bot from the outside world.

Johannes I think the external trigger thing is slightly different from the reminder though?

Sorry, I mean both reminders and external triggers cause the same problem and should therefore both trigger intents instead of actions; but I focused on reminders so far, because we need this in Carbon Bot.

reminder is unpredictable action, therefore we need to create separate stories that start with it. If reminder would be similar to intent, it could be part of any regular story

in xy there were no ML policy, we created separate stories for reminders, and created augmemo to recall them (that’s how aug memo was born)

I think Johannes idea makes total sense, but we should still support both cases, because sometimes having an action is more convenient, especially if it forgets itself

no i think what ella is saying that there’s no need to keep the option of just having an action? Because you could equally just do UserUtteranceReverted with the new set up of itnent/action

I think this makes a lot of sense. to me it reflects the way we currently suggest to have the bot start conversations: send hidden get started payload — this is essentially an external event saying :“the user opened the widget”
being able to include it in the stories and learn on it would allow injection of a mini-conversation inside a story, which I think continental is going to need.

we still have to convince @wochinge to let me put it in the socket channel though 😃the annoying thing about external actions is you can’t test them in the command line channel or the rest channel (unless you’re consuming the requests somewhere else) (convo for a different thread though probably)

Another reason to implement this — AFAIK currently external actions only allow you to name which action should be run, you can’t send over any info that you might need within the action. being able to send message metadata in the WorldMessaged solves this, which stops you from having to create a different action for every possible case of info you’re trying to send

@JEM-Mosig
Copy link
Author

@tmbo @wochinge @akelad @erohmensing It would be great to get your feedback on my current plan to introduce external events. If it's fine, I'll go ahead implementing it:

  • Introduce WorldMessaged(Event) class similar to UserUttered
  • WorldMessaged contains a message name message and a list of entities, but no utterance
  • Introduce a new kind of story line that starts with a !, indicating an external or reminder event
  • The syntax is otherwise identical to user messages, e.g.
! remind{"remind_type": "stove"}
  - utter_turn_off_stove
  • Here the WorldMessaged event with message remind and entity remind_type is triggered and the bot replies with utter_turn_off_stove
  • This requires changes in the routines that parse story and domain files, respectively
  • Reminders always trigger WorldMessaged, and triggering actions will be no longer supported
  • MessageProcessor._predict_and_execute_next_action will be changed to take output_channel as an argument instead of message: UserMessage, since only the output channel from the message is ever used

I am not sure what I'll have to do about external triggers.

@akelad
Copy link
Contributor

akelad commented Nov 12, 2019

@JEM-Mosig my initial thought is can we call the WorldMessaged something more intuitive? E.g. ExternalEvent or something - not sure if that's the right one either, but WorldMessaged sounds a bit weird to me

@JEM-Mosig
Copy link
Author

JEM-Mosig commented Nov 13, 2019

@akelad Sure, I am open to suggestions. I came up with WorldMessaged to keep it in line with UserUttered, SlotSet, and BotUttered. ExternalEvent kind of breaks this pattern. But it does sound better...

@wochinge
Copy link
Contributor

Could we have ExternalEvents but treat them the same as intents in the stories, e.g. we define some internal intents like /reminder ? I think we shouldn't make story files more complex without a very good reason.

@tmbo
Copy link
Member

tmbo commented Nov 20, 2019

Well but we need some way to distinguish between the events

@JEM-Mosig
Copy link
Author

Like intents, external events will have to be listed in the domain file. We could require their names to be distinct from the intent names.

@JEM-Mosig
Copy link
Author

I am implementing a draft now. One thing that I am unsure about is the reversed actions. I'll introduce an ExternalEventReverted event for now, but it might make sense to not do that and instead revert external events with UserUtteredReverted as well. That might be confusing, however. What do you think?

@wochinge
Copy link
Contributor

wochinge commented Dec 2, 2019

Do we actually need it right now?

@JEM-Mosig
Copy link
Author

We need something like ExternalEventReverted for interactive learning at least.

@JEM-Mosig
Copy link
Author

Adding a new ExternalEvent class with a corresponding section in the domain file, requires a lot of code changes. We have to decide if it is worth it.

In favour of the separate ExternalEvent class is that it is conceptually clearer. I thought later we could introduce special events, such as "chat window opened", etc., but that might not even be possible in all cases.
The alternative would be to go with the solution I had used in the screenshots above, where the reminder triggers an intent that is not displayed, and implement the same for other external events.

What do you think? @tmbo @wochinge @akelad

@wochinge
Copy link
Contributor

wochinge commented Dec 6, 2019

The alternative would be to go with the solution I had used in the screenshots above, where the reminder triggers an intent that is not displayed, and implement the same for other external events.

What about:

  • using the latter approach (using special intents instead of a new intent)
  • adding the option to configure an intent in the domain as is_external. The is_external value then would be added to the UserUttered event and frontend could decide not to show this
  • adding default, pre-defined intents for Rasa things (e.g. an intent /reminder) to the mapping policy / domain

@wochinge
Copy link
Contributor

wochinge commented Dec 9, 2019

Summary of call

  • No external event class
  • Endpoint POST "/conversations/<conversation_id>/trigger/<intent>"
  • is_external metadata for these events (Rasa X UI can hide them then)

@tmbo We would create a new endpoint to trigger intents. We could also

  • use the rest inputchannel, but it would be weird to send the output of the actions to a different user and to set the is_external metadata
  • use the appendEvents endpoint, but that doesn't trigger prediction .

Is that ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. 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.

4 participants