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

Default slots #7529

Merged
merged 11 commits into from
Dec 14, 2020
Merged

Default slots #7529

merged 11 commits into from
Dec 14, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Dec 11, 2020

Proposed changes:

  • add default slots upon domain creation to ensure fingerprints are created correctly (previously there was problem because we compared a domain without default slots with a domain which already contained the default slots - the test added to test_model visualizes this)
  • deprecate add_categorical_slot_default_value, add_requested_slot, add_knowledge_base_slots. Their internal versions are automatically called when the domain is created
  • don't persist the default value for categorical slots. We add it when the domain is loaded.

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)

@wochinge wochinge force-pushed the default-slots branch 2 times, most recently from 70315fe to 7e29de1 Compare December 14, 2020 11:13
@@ -8,3 +8,4 @@ pipeline:
# https://rasa.com/docs/rasa/policies
policies:
- name: MemoizationPolicy
- name: RulePolicy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was needed to work with the form which I added to the default domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the presence of the RulePolicy in turn led to fallback, which is why I had to add the new story

rasa/shared/core/slots.py Show resolved Hide resolved
tests/core/test_model.py Show resolved Hide resolved
tests/core/test_model.py Outdated Show resolved Hide resolved
tests/shared/core/test_domain.py Outdated Show resolved Hide resolved
@joejuzl
Copy link
Contributor

joejuzl commented Dec 14, 2020

Looks good! 🚀

@wochinge
Copy link
Contributor Author

@joejuzl I suspect merge conflicts when merging 2.1.x to master. How about I handle the 2.1.x to master merge (will request your review) which you'd have done otherwise for the release?

@joejuzl
Copy link
Contributor

joejuzl commented Dec 14, 2020

@wochinge that would be great - thank you

@wochinge wochinge marked this pull request as ready for review December 14, 2020 16:59
@wochinge wochinge requested review from a team and joejuzl December 14, 2020 16:59
@rasabot rasabot merged commit 5684e69 into 2.1.x Dec 14, 2020
@rasabot rasabot deleted the default-slots branch December 14, 2020 17:12
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.

3 participants