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.

Sign up for GitHub

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

Jump to bottom

Rules states should not include the initial values of slots. #8161

Merged
merged 13 commits into from
Mar 12, 2021
Merged

Rules states should not include the initial values of slots. #8161

merged 13 commits into from
Mar 12, 2021

Conversation

joejuzl
Copy link
Contributor

@joejuzl joejuzl commented Mar 10, 2021

Proposed changes:
Fixes #7450

When calculating tracker states from rules we were including the initial value of slots.
This meant that the rule only matched when a slot had that initial value.
This change omits initial values of slots from the rule states.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@joejuzl joejuzl requested review from samsucik, wochinge and a team and removed request for a team March 10, 2021 09:36
samsucik
samsucik suggested changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@samsucik samsucik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joejuzl, I know this bug was annoying to users and it's good to see it go.

I'm not familiar with all the code you've touched but the changes look good to me and I'm gonna trust the newly added test cases 🙂

joejuzl and others added 5 commits March 11, 2021 16:47
@joejuzl @samsucik
Apply suggestions from code review
95025d3 
docstring changes

Co-authored-by: Sam Sucik <s.sucik@rasa.com>
@joejuzl @samsucik
Apply suggestions from code review
d85a3f4 
docstring updates in unchanged code

Co-authored-by: Sam Sucik <s.sucik@rasa.com>
@joejuzl
update tests
e7f76ab
@joejuzl
remove unnecessary listen action
1295b76
@joejuzl
Merge branch 'main' into 7450-rule_applicability_depends_on_slots_ini…
6c8dab6 
…tial_value
wochinge
wochinge reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concise change! 💯

It feels a bit brute force which made me wonder how an ideal solution could look like: One idea could be to have a SlotFeaturizer interface with a specific implementation for the RulePolicy's use case. The RulePolicy would then pass that to the TrackerFeaturizer which would than use that to featurize the slots. It's currently a bit unclear to me why we need tracker.past_states if it just calls the Domain 🤷🏻 Just to be clear: I'm fine with the current solution but in my opinion we should refactor as soon as we start passing a second flag down the line.

@joejuzl
Copy link
Contributor Author

joejuzl commented Mar 11, 2021

Concise change! 💯
It feels a bit brute force which made me wonder how an ideal solution could look like: One idea could be to have a SlotFeaturizer interface with a specific implementation for the RulePolicy's use case. The RulePolicy would then pass that to the TrackerFeaturizer which would than use that to featurize the slots. It's currently a bit unclear to me why we need tracker.past_states if it just calls the Domain 🤷🏻 Just to be clear: I'm fine with the current solution but in my opinion we should refactor as soon as we start passing a second flag down the line.

Yep very good points. Having ways to featurize differently based on context would nice, but yeah let's see how it develops.

joejuzl and others added 2 commits March 11, 2021 17:47
@wochinge
Copy link
Contributor

👍🏻

@samsucik Feel free to approve once you think Joe adressed all your changes. Engineering wise it's ready to go from my side.

samsucik
samsucik approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@samsucik samsucik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@joejuzl joejuzl enabled auto-merge (squash) March 12, 2021 10:15
@joejuzl joejuzl merged commit 2aade32 into main Mar 12, 2021
@joejuzl joejuzl deleted the 7450-rule_applicability_depends_on_slots_initial_value branch March 12, 2021 13:44
Ghostvv pushed a commit that referenced this pull request Mar 12, 2021
@joejuzl @samsucik
@wochinge @Ghostvv
Rules states should not include the initial values of slots. (#8161)
20e5faf 
* Rules states should not include the initial values of slots.

* changelog entry

* Apply suggestions from code review

docstring changes

Co-authored-by: Sam Sucik <s.sucik@rasa.com>

* Apply suggestions from code review

docstring updates in unchanged code

Co-authored-by: Sam Sucik <s.sucik@rasa.com>

* update tests

* remove unnecessary listen action

* Apply suggestions from code review

Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>

* black

Co-authored-by: Sam Sucik <s.sucik@rasa.com>
Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>
This was referenced Aug 31, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wochinge wochinge wochinge left review comments

@samsucik samsucik samsucik approved these changes

Assignees
No one assigned
Labels
tools:clear-poetry-cache-unit-tests Clear poetry cache for the unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule applicability depends on slot values when initial_value is used
3 participants
@joejuzl @wochinge @samsucik