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

unfeaturized slots #6817

Merged
merged 26 commits into from
Oct 1, 2020
Merged

unfeaturized slots #6817

merged 26 commits into from
Oct 1, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Sep 28, 2020 •
edited
Loading

Proposed changes:

  • fix Add unfeaturized flag to slots #6809
  • add a flag influence_conversation to each slot which determines whether the slot should featurized. Each slot type is featurized by default to avoid that existing bots break. Only UnfeaturizedSlots are not featurized by default.
  • deprecate UnfeaturizedSlot as users should use the flag instead
  • replace usages of UnfeaturizedSlot with equal usages of unfeaturized, but not deprecated slots
  • introduce a new slot type AnySlot which can store anything but is required to be unfeaturized. Users have to implement custom slots if they want to featurize an AnySlot
  • raise exceptions if somebody tries to make UnfeaturizedSlot or AnySlot featurized
  • removed DataSlot which was unused and doesn't make sense as well
  • adapt examples to use influence_conversation: False instead of using the deprecated unfeaturized slot
  • adapt slot documentation and documentation which refers to unfeaturized slots
  • add migration guide for unfeaturized slots

Todos

  • agree on a flag name
  • docs (migration guide, slots docs)

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 unfeaturized-slots branch 3 times, most recently from e5d860b to f45a0a6 Compare September 29, 2020 09:47
wochinge
wochinge commented Sep 29, 2020
View reviewed changes
docs/docs/domain.mdx Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

@akelad Who from CSE would be best to help me with the documentation? I did a draft for the slots documentation, next step is the migration guide then.

@wochinge wochinge marked this pull request as ready for review September 29, 2020 12:41
@akelad
Copy link
Contributor

akelad commented Sep 29, 2020

@wochinge just to clarify, you're gonna write the migration guide? General process is person building the feature writes the docs, CSE reviews

@wochinge
Copy link
Contributor Author

wochinge commented Sep 29, 2020 •
edited
Loading

@akelad Yep, all done but I still need review + support as especially the changes for the slots docs were a bit more. I think I know the general process by now 😄

@akelad
Copy link
Contributor

akelad commented Sep 29, 2020

sorry was just unclear on what you meant with your comment, sounds good!

indam23
indam23 suggested changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@indam23 indam23 left a comment

Choose a reason for hiding this comment

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

Thanks for writing the docs 🚀 Just a few suggestions for clarity.

docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Show resolved Hide resolved
docs/docs/domain.mdx Show resolved Hide resolved
docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/forms.mdx Outdated Show resolved Hide resolved
docs/docs/forms.mdx Outdated Show resolved Hide resolved
docs/docs/migration-guide.mdx Show resolved Hide resolved
docs/docs/unexpected-input.mdx Outdated Show resolved Hide resolved
ricwo
ricwo approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

Should we add a special warning to resolve_by_type so that users are alerted of the removal of the unfeaturized property? Right now all they'll see is that that slot doesn't exist, but this can be confusing when you haven't read the changelog and/or migration guide.

Other than that, great work 💯

docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Outdated Show resolved Hide resolved
docs/docs/domain.mdx Outdated
weather, and you *don't* know their home city, you will have to ask
them for it. A `text` slot only tells Rasa Core whether the slot
them for it. A [text slot](domain.mdx#text-slot) only tells Rasa Core whether the slot
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this one:

Suggested change
them for it. A [text slot](domain.mdx#text-slot) only tells Rasa Core whether the slot
them for it. A [text slot](domain.mdx#text-slot) only tells Rasa whether the slot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the whole sentence 🙈

docs/docs/domain.mdx Outdated Show resolved Hide resolved
rasa/shared/core/slots.py Outdated Show resolved Hide resolved
rasa/shared/core/slots.py Outdated Show resolved Hide resolved
tests/shared/core/test_slots.py Outdated Show resolved Hide resolved
tests/shared/core/test_slots.py Show resolved Hide resolved
rasa/shared/core/domain.py Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

Thanks for your blazing fast reviews ❤️

wochinge and others added 7 commits September 30, 2020 11:08
@wochinge
polish ✨
5f16562
@wochinge
persist influence_conversation
fc2599a
@wochinge @ricwo @indam23
improve phrasing
1144b5e 
Co-authored-by: ricwo <20581300+ricwo@users.noreply.github.com>
Co-authored-by: Melinda Loubser <32034278+melindaloubser1@users.noreply.github.com>
@wochinge
clarify code
2b76f30
@wochinge
add hint for subclassing 'Slot' to error message
f0f5733
@wochinge
re-arrange slot behavior section
23a98b3
@wochinge
add test for slot persistence
f4b6f87
@wochinge
Copy link
Contributor Author

@ricwo Not sure I'm understanding this one

Should we add a special warning to resolve_by_type so that users are alerted of the removal of the unfeaturized property?

There is a deprecation warning if you use an UnfeaturizedSlot.

@ricwo
Copy link
Contributor

ricwo commented Sep 30, 2020

@wochinge oh of course - the class still exists. please ignore my comment 👍

@wochinge
Copy link
Contributor Author

it made me reconsider the content of the deprecation warning which was helpful 👍

@rasabot rasabot merged commit 91a5b5f into master Oct 1, 2020
@rasabot rasabot deleted the unfeaturized-slots branch October 1, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricwo ricwo ricwo approved these changes

@indam23 indam23 indam23 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add unfeaturized flag to slots
5 participants
@wochinge @akelad @ricwo @indam23 @rasabot