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

hide only rule dialogue turns #7701

Merged
merged 110 commits into from
Mar 22, 2021
Merged

hide only rule dialogue turns #7701

merged 110 commits into from
Mar 22, 2021

Conversation

Ghostvv
Copy link
Contributor

@Ghostvv Ghostvv commented Jan 8, 2021

Proposed changes:

originally I had 2 ideas: operate on states or use forgetting functionality in tracker.applied_events through specialized events. In the end I used the mix of these approaches.

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)

@Ghostvv Ghostvv requested review from joejuzl, wochinge, dakshvar22 and a team January 8, 2021 13:09
docs/docs/rules.mdx Outdated Show resolved Hide resolved
changelog/7701.improvement.md Outdated Show resolved Hide resolved
docs/docs/rules.mdx Outdated Show resolved Hide resolved
Ghostvv and others added 3 commits January 11, 2021 16:28
Co-authored-by: Ella Rohm-Ensing <[email protected]>
Co-authored-by: Ella Rohm-Ensing <[email protected]>
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Jan 11, 2021

@wochinge could you please take a look if we can remove forgetting of TwoStageFallback rules

dakshvar22
dakshvar22 previously approved these changes Jan 11, 2021
Copy link
Contributor

@dakshvar22 dakshvar22 left a comment

Choose a reason for hiding this comment

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

Amazing job on this§1 💯 Just a couple of minor comments from my side.

rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/shared/core/domain.py Outdated Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Show resolved Hide resolved
@Ghostvv Ghostvv added the tools:clear-poetry-cache-unit-tests Clear poetry cache for the unit tests label Jan 20, 2021
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.

I need a bit more time to digest this so here is my input on this so far:

  • If users turn of _check_for_contradictions then this wouldn't work only longer, correct?
  • Wouldn't it be better to warn the user that they should add a story so that the ML policies can learn to perform the transition after a rule application instead of completely blanking out the rule?
  • Could we also to this without the additional event and simply remove all events before an ActionExecuted with policy=="RulePolicy" until the previous action?

rasa/core/featurizers/tracker_featurizers.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/policy.py Outdated Show resolved Hide resolved
rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Jan 20, 2021

Wouldn't it be better to warn the user that they should add a story so that the ML policies can learn to perform the transition after a rule application instead of completely blanking out the rule?

no, because you'd need to add combinatorial number of rules into stories, which is unnecessary if you can hide rules

Could we also to this without the additional event and simply remove all events before an ActionExecuted with policy=="RulePolicy" until the previous action?

no, because not all rules should be ignored, only the ones that are not present in the stories, also not all slot sets should be ignored

Ghostvv and others added 2 commits January 20, 2021 19:53
@wochinge
Copy link
Contributor

But the domain currently contains all the shared information already, no?

Actually, they can be different after each retrain

What do you mean? You mean in case you add another story?

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Mar 15, 2021

What do you mean? You mean in case you add another story?

yes

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Mar 15, 2021

also, updating domain retrospectively after training doesn't feel right

@Ghostvv Ghostvv requested review from wochinge and removed request for joejuzl March 15, 2021 12:50
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Mar 18, 2021

@wochinge could you give it a review? is it ready to merge?


@property
def featurizer(self):
"""Returns the policy's featurizer."""
return self.__featurizer

def set_rule_only_data(self, rule_only_data: Dict[Text, Any]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using something which generalizes better?

def set_shared_policy_state(**kwargs: Any) -> None

Advantages:

  • no deprecation is required if we want to pass more information / different information
  • users can use this for their own policies (e.g. setting some shared state originating from their policies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

users wouldn't be able to use it, since ensemble will only pass rule_only_data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed the method

rasa/core/policies/policy.py Show resolved Hide resolved
rasa/shared/core/domain.py Outdated Show resolved Hide resolved
rasa/shared/core/generator.py Outdated Show resolved Hide resolved
rasa/shared/core/trackers.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
@Ghostvv Ghostvv removed the tools:clear-poetry-cache-unit-tests Clear poetry cache for the unit tests label Mar 19, 2021
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.

👍🏻

@Ghostvv Ghostvv enabled auto-merge (squash) March 22, 2021 09:06
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Mar 22, 2021

@wochinge do you have an idea why test docs fail? I tried restarting the docs test several times

@wochinge
Copy link
Contributor

due to #8272

@Ghostvv Ghostvv merged commit 8258c55 into main Mar 22, 2021
@Ghostvv Ghostvv deleted the hide_rules branch March 22, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unfeaturize rule snippets for inference through TED
6 participants