-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Rule policies will not fire if unrelated featurized slots are set #9302
Comments
Example repo of this issue is here |
I'm trying to understand how this issue might affect another bot and created a more-help branch which adds two more featurized slots and test stories that demonstrate the problem. The main branch has a single featurized categorical slot called In this branch, I've added another featurized categorical slot called test_stories.yml:- story: goodbye no slots
steps:
- user: |
bye-bye!
intent: goodbye
- action: utter_goodbye
- story: goodbye - num_appts slot
steps:
- slot_was_set:
- num_appts: multiple
- user: |
bye-bye!
intent: goodbye
- action: utter_goodbye
- story: goodbye acct_type slot
steps:
- slot_was_set:
- acct_type: basic
- user: |
bye-bye!
intent: goodbye
- action: utter_goodbye failed_test_stories.yml:version: "2.0"
stories:
- story: goodbye - num_appts slot (tests/test_stories.yml)
steps:
- slot_was_set:
- num_appts: multiple
- intent: goodbye
- action: utter_goodbye # predicted: action_default_fallback |
@rgstephens before someone from Research picks this up properly as a sprint task, I quickly looked into it and noticed one difference in the way the two slots are handled -- which explains the behaviour you're observing, though the question now becomes why the 2 slots are handled differently. The difference is that in the trained RulePolicy, the rules explicitly condition only on the "[{\"prev_action\": {\"action_name\": \"action_listen\"}, \"slots\": {\"num_appts\": [1.0, 0.0, 0.0, 0.0]}, \"user\": {\"intent\": \"bot_challenge\"}}]": "utter_iamabot", So, the question for whoever investigates this further is: Why would one featurised slot appear in the rules, and another one wouldn't? In terms of how can a slot even get ignored (i.e. not included in a rule), one example is the already mentioned fix which excludes explicitly defined |
for the research teams backlog meeting on friday: this seems like relatively high priority to be addressed |
Yes, I second Akela's comment ☝️ - PoC customer is affected by it and their engagement ends in about a month - outcome of the PoC may depend on having this issue fixed or at least worked on. |
Here is another aspect of this issue - this rule and story are found to conflict:
This is a true conflict iff RulePolicy ignores featurized slots that aren't specified in a rule. If it does consider them (which it seems to, at least sometimes), then there should be no conflict, since the rule is equivalent to the following rule with which no conflict is found:
A user pointed out that the reason this conflict is found is that this line: rasa/rasa/core/policies/rule_policy.py Line 810 in 93d9983
Only uses keys that actually appear in the rule to check equality of a rule and story state - since there are no slots in the rule above, the slots are not even included in consideration for conflict checking. |
I'm one of the customers experiencing the issue and I think the core issue (or at least the part that's causing me problems) is actually caused by the fact that the featurized categorical slot has an allowable value of |
I'll now dig into this issue, especially considering the interesting behaviour that @provwilliamnagy described. As for the observation that @melindaloubser1 made, I wanted to make a not about that first. I think it takes us back to an assumption made when designing rules: That if a rule doesn't explicitly mention a slot, then it should apply regardless of what that slot is set to. There are 3 cases I'd like to distinguish when discussing this assumption:
Based on the last 2 cases and what I said about them, I think the conflict reported by @melindaloubser1 is actually correct (even though it's a special kind of conflict that deserves to be communicated more clearly, or maybe we need clearer guidance in the docs to prevent it). Why? Well, we have a rule: - rule: Say 'I am a bot' anytime the user challenges
steps:
- intent: bot_challenge
- action: utter_iamabot By not including any slots in the rule, we say "this context is slot-agnostic and we always want this rule to apply". (If the context wasn't slot-agnostic, we would've included the slot in the rule!) But then we have the story: - story: bot challenge with slot
steps:
- intent: bot_challenge
entities:
- name: "amy"
- slot_was_set:
- name: 'amy'
- action: utter_iamabot_name And this story basically says "no, no, in this context there is actually a slot which matters and based on its value we want to take a specific action ( So, conceptually, the rule and the story are going against each other. @melindaloubser1 what do you say? I'm very curious, as I feel that we're going into quite philosophical questions here 😂 |
I agree that it can be a valid conflict
The issue is that seems not to be true in all cases (according to other examples in this issue) If we can fix it so that it is as you describe in all cases and make it crystal clear in documentation, then this conflict is a true positive and not a problem. Assuming that that is intended behaviour, is the logic here of ignoring keys that aren't found in a rule then correct? rasa/rasa/core/policies/rule_policy.py Line 810 in 93d9983
|
@melindaloubser1 I think we're on the same page. The outstanding issue is featurised slots sometimes getting embedded in rules even if they're not present in the rule as it is written. Once that's fixed, we can make things clear in the docs. As for that code snippet, I believe it is correct and does what we want (though it makes some of the very core logic rather implicit and deserves to be at least documented much better). |
@provwilliamnagy thanks for the very helpful pointer, indeed a possible slot value of |
I investigated our options because it wasn't immediately clear to me where and how the bug should be fixed. As I was thinking it through (which was taking quite some time), I also wrote a fix (which was very quick). My thoughts are now captured in this PR: #9475. It fixes the specific issue pointed out by @provwilliamnagy (feel free to check it out, everything should work for you if you install Rasa Open Source from that branch) and takes us to the state where all the examples provided by @rgstephens work nicely. The PR also proposes more clarity around unset slots and the use of |
@samsucik are you also planning on addressing this in PR #9475:
I just pulled your branch and ran it on @rgstephens slots:
num_appts:
type: categorical
auto_fill: true
initial_value: single
influence_conversation: true
values:
- none
- single
- multiple which causes rule definitions to pick up this initial value implicitly and therefore fail to ignore the Is this the case you are referring to of slots getting implicitly embedded in rules? Are there other cases? |
@kedz the PR (#9475) is meant to fix the known cases of featurised slots making it into rules when they shouldn't. From the examples we've got, this seems to be limited to the case when a categorical slot's allowed value is As for your observation with the initial value, this is very interesting as it should've been fixed by #8161 🤔 I'll look into it now. |
@kedz I'm also seeing the incorrect behaviour with |
Fyi the bug report for the |
@samsucik should this be closed? |
@tmbo yes (the fixing patch release is out as of a few minutes ago). But, since this issue generated quite a lot of followups, I'll have another pair of eyes look at it before closing it. |
sounds great 🔥 |
Product feedback submission from @rgstephens
Who is the feedback from?
Multiple customers
What is the feedback?
Rule policies will not fire if unrelated featurized slots are set. This behavior is counterintuitive and not documented.
What problem are they trying to solve?
A simple rule like this should work if a featurized slot is set:
Repo with more details
Slack thread
Recommendation
Possible Inelegant Workaround
Possible Solutions
Definition of Done
In order of urgency:
Instead of the above steps, @samsucik carried out an investigation to pin-point the bug and, while doing so, proposed a simple fix (implemented in #9475), which has been verified to fix the issue on all examples provided by @rgstephens. The discussion inspired related improvements to our docs and another bug report.
@samsucik to coordinate who is doing what on this issue
@kedz assigned reviewer
The text was updated successfully, but these errors were encountered: