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

Add unfeaturized flag to slots #6809

Closed
2 tasks
wochinge opened this issue Sep 28, 2020 · 8 comments · Fixed by #6817
Closed
2 tasks

Add unfeaturized flag to slots #6809

wochinge opened this issue Sep 28, 2020 · 8 comments · Fixed by #6817
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR

Comments

@wochinge
Copy link
Contributor

wochinge commented Sep 28, 2020

Motivation

Slots themselves cause confusion in another way -- users may define the type based on what they hold (e.g. “text”) but not include them in their stories because they shouldn’t affect the flow -- it’s common that users don’t understand that setting the type to text will actually featurize the slot, leading to unpredictable behavior.

Suggested Solution

  1. Add a flag unfeaturized: true / false to the configuration of each slot in the domain. Return the slots features in as_feature depending on the value of the unfeaturized flag

    slots:
      os:
        type: categorical
        values:
        - Windows
        - MacOS
        - Linux
        unfeaturized: true
     # this would previously been
     # os2:
       # type: unfeaturized

    Open questions:

    • What should be the default value for unfeaturized? I'd vote for false because we will otherwise breaking existing bots.
  2. Deprecate UnfeaturizedSlot
    Open questions:

    • Should we introduce a slot type Any instead which can hold arbitrary values? I'd vote for yes. We can introduce a slot type Any which explicitly throws an error when used with unfeaturized: false. If users want to use an Any slot with unfeaturized: false, they need to implement their own slot type
  3. Check whether / how this affects Rasa X / Rasa SDK

@wochinge wochinge added type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR area:rasa-oss 🎡 Anything related to the open source Rasa framework labels Sep 28, 2020
@wochinge wochinge added this to the 2.0 Rasa Open Source milestone Sep 28, 2020
@wochinge
Copy link
Contributor Author

@akelad @tmbo Please double check

We currently don't touch the slot values for Unfeaturized. Means if you feed a dictionary (e.g. as part of a custom action), then it will stay a dictionary

@wochinge
Copy link
Contributor Author

Ok, let's featurize all slots by default (except the old UnfeaturizedSlot as we will break all existing bots otherwise.

@tmbo
Copy link
Member

tmbo commented Sep 28, 2020

Ok, let's featurize all slots by default (except the old UnfeaturizedSlot as we will break all existing bots otherwise.

Yes this sounds reasonable.

Should we introduce a slot type Any instead which can hold arbitrary values? I'd vote for yes.

I'd agree. The unfeaturized slot type should stay and throw a deprecation warning.

Regarding the naming of the flag:

  • I think it is easier to understand if a flags name is not negated, so featurized instead of unfeaturized
  • Maybe we can come up with a better naming, e.g. influences_conversation or influences_prediction or something that is slightly less ml heavy speech?

@wochinge
Copy link
Contributor Author

Both valid points 👍 I'd personally prefer imperative style and affect (e.g. affect_conversation) but maybe @erohmensing or @melindaloubser1 can help us out as native speakers.

@wochinge wochinge mentioned this issue Sep 28, 2020
6 tasks
@indam23
Copy link
Contributor

indam23 commented Sep 29, 2020

I like "influence_conversation"; English speakers can never get their affects and effects sorted out anyway.

@wochinge
Copy link
Contributor Author

Sounds good. Gonna go with that one then 👍

@wochinge
Copy link
Contributor Author

@melindaloubser1 Just stumbled across this line:

raise NotImplementedError(

It doesn't make sense to raise a NotImplementedError here. None except the CategoricalSlot currently implement this. Means this would explode for all other of the standard slots. I think it should rather be pass. any objections?

@wochinge
Copy link
Contributor Author

pass also doesn't make sense. I'd just remove this from the parent class Slot, ok?

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 type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants