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

Rule policies will not fire if unrelated featurized slots are set #9302

Closed
4 tasks
TyDunn opened this issue Aug 9, 2021 · 19 comments
Closed
4 tasks

Rule policies will not fire if unrelated featurized slots are set #9302

TyDunn opened this issue Aug 9, 2021 · 19 comments
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/ml/policies Issues focused around rasa's dialogue management policies cse-issues type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors.

Comments

@TyDunn
Copy link
Contributor

TyDunn commented Aug 9, 2021

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:

- rule: greet
 steps:
 - intent: greet
 - action: utter_greet

Repo with more details

Slack thread

Recommendation

  • Assign two researcher to this issue

Possible Inelegant Workaround

  • Add a rule for every possible slot state

Possible Solutions

  • Make the state of slot featurization explicit in the rules (e.g. none or any)

Definition of Done
In order of urgency:

  • See whether suggested workaround will resolve immediate customer problem
  • Understand customer context: who does this affect?
  • Identify consequences: what would change if this suggested behavior change were implemented?
  • Identify possible alternative next steps to be refined and scoped later, likely to include more explicit definition of rule behavior

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

@TyDunn TyDunn added type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors. area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/ml/policies Issues focused around rasa's dialogue management policies labels Aug 9, 2021
@rgstephens
Copy link
Contributor

Example repo of this issue is here

@rgstephens
Copy link
Contributor

rgstephens commented Aug 11, 2021

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 num_appts where the value is set in a custom action and the response to the goodbye intent is always incorrect if num_appts is set to any value.

In this branch, I've added another featurized categorical slot called acct_type but I can't get the problem to exhibit itself with this slot. With the test cases below, the num_appts test consistently fails but not acct_type.

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

@samsucik
Copy link
Contributor

@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 num_appts slot's initial value, but not on acct_type. I.e. if you unzip the trained model and look into the model_name/core/policy_1_RulePolicy/rule_policy.json file, you'll see rules like this:

"[{\"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 initial_values of slots from rules. Here, however, we're talking about implicit initial values -- even an initially unset slot will have some default (initial) featurisation.

@akelad
Copy link
Contributor

akelad commented Aug 17, 2021

for the research teams backlog meeting on friday: this seems like relatively high priority to be addressed

@z-bodikova
Copy link

z-bodikova commented Aug 17, 2021

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.

@indam23
Copy link
Contributor

indam23 commented Aug 18, 2021

Here is another aspect of this issue - this rule and story are found to conflict:

rules:
- rule: Say 'I am a bot' anytime the user challenges
  steps:
  - intent: bot_challenge
  - action: utter_iamabot

stories:
- story: bot challenge with slot
  steps:
  - intent: bot_challenge
    entities:
    - name: "amy"
  - slot_was_set:
    - name: 'amy'
  - action: utter_iamabot_name

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:

rules:
- rule: Say 'I am a bot' anytime the user challenges
  condition:
  - slot_was_set:
     - name: null
  steps:
  - intent: bot_challenge
  - action: utter_iamabot

A user pointed out that the reason this conflict is found is that this line:

for key, value_from_rules in rule_sub_state.items():

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.

@provwilliamnagy
Copy link

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 none (in my case, the allowed values are none, single, multiple). I thought those were being treated as strings but perhaps something in the Python code is confusing it and treating it as None even though it was specified in the YAML. If I rename that value in the category to something like nada, it doesn’t appear in the compiled rules…

@samsucik
Copy link
Contributor

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:

  1. I think this assumption makes sense for slots that don't influence the conversation.
  2. Similarly, we can think of slots that do influence the conversation, but aren't related to the rule in question, and so the assumption holds (e.g., a intent: chitchat -> action: utter_chitchat rule might need to apply regardless of the value of a featurised onboarding_flow_stage slot).
  3. Finally, there are then also slots that influence the conversation and are relevant to the rule. However, then, these slots need to be written explicitly in the rule -- we'll create separate versions of the rule for different values of the slot.

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 (utter_iamabot_name)".

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 😂

@indam23
Copy link
Contributor

indam23 commented Aug 27, 2021

I agree that it can be a valid conflict

iff RulePolicy ignores featurized slots that aren't specified in a rule.

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?

for key, value_from_rules in rule_sub_state.items():

@samsucik
Copy link
Contributor

@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).

@samsucik
Copy link
Contributor

samsucik commented Aug 30, 2021

@provwilliamnagy thanks for the very helpful pointer, indeed a possible slot value of none is being treated the same as the initial value of the slot (the Pythonic None) during slot featurisation (code here). However, fixing this might actually happen elsewhere, not this far down the line in the code. Because, if a slot is unset (i.e. its value is the Pythonic None), we might want to not even try to featurise it. I'm looking into it now.

@samsucik
Copy link
Contributor

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 None/null as categorical slot values.

@kedz
Copy link
Contributor

kedz commented Aug 30, 2021

@samsucik are you also planning on addressing this in PR #9475:

The outstanding issue is featurised slots sometimes getting embedded in rules even if they're not present in the rule as it is written.

I just pulled your branch and ran it on @rgstephens more-help branch but added an initial value to domain like so:

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 num_appts value at test time.

Is this the case you are referring to of slots getting implicitly embedded in rules? Are there other cases?

@samsucik samsucik assigned kedz and unassigned tttthomasssss Aug 31, 2021
@samsucik
Copy link
Contributor

samsucik commented Aug 31, 2021

@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 none (or NoNe, NONE, etc.). And this is what #9475 fixes.

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.

@samsucik
Copy link
Contributor

@kedz I'm also seeing the incorrect behaviour with initial_value set. Having done some early investigation, I think it's separate from the bug discussed in this issue, follows from insufficient testing of #8161, and should be dealt with separately. I'll create a bug report for it. Then, I'll focus on getting #9475 ready for a review.

@samsucik
Copy link
Contributor

Fyi the bug report for the initial_value issue now lives here: #9483 .

@tmbo
Copy link
Member

tmbo commented Sep 9, 2021

@samsucik should this be closed?

@samsucik
Copy link
Contributor

samsucik commented Sep 9, 2021

@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.

@tmbo
Copy link
Member

tmbo commented Sep 9, 2021

sounds great 🔥

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 area:rasa-oss/ml/policies Issues focused around rasa's dialogue management policies cse-issues type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

No branches or pull requests