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

Allow forms without default slot mappings / only custom slots mappings #7068

Closed
wochinge opened this issue Oct 20, 2020 · 17 comments · Fixed by #7171
Closed

Allow forms without default slot mappings / only custom slots mappings #7068

wochinge opened this issue Oct 20, 2020 · 17 comments · Fixed by #7171
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 Oct 20, 2020

Currently, you need to add at least one slot mapping to your form in the domain. Otherwise, it will choose the Rasa Open Source 1.x FormAction for execution (see here)

We need to detect the deprecated usage of old forms differently and allow that users can define a form like the following:

forms:
   my_form:
     # no slot mappings
     # all slots are extracted in the custom validation action
@wochinge wochinge added priority:high 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 labels Oct 20, 2020
@wochinge wochinge added this to the 2.1 Rasa Open Source milestone Oct 20, 2020
@wochinge
Copy link
Contributor Author

wochinge commented Nov 3, 2020

@Ghostvv @akelad

Current Situation
In case there are no slot mappings defined for a form, we currently assume that they want to use the deprecated Rasa Open Source 1 FormAction. We have to replace this check with a different one in order to allow forms without any slot mappings in the domain.

Solutions
I gathered two solutions which make sense in my opinion:

  1. Allow users to specify a key use_deprecated_form_action for each form. This has more implications for Rasa X and code wise, but would be very explicit.
  2. Allow users to override the form by specifying a custom action with the name of the form. This would be more general and would require less changes. However, it might be a bit unintuitive for users.

In general I'd prefer option 2 as it seems more "Rasa-style" and blends with how we override other "default" actions. Any strong opinions?

@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

the first version still technically breaks compatibility? I thought the idea is to allow users to train/run old 1.x bots without any changes to the files

@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

I'm not sure, I understand the solution 2, do you mean that if there is an action with the same name, we override the default? I thought we already do it, at least we did for action_listen in some old version

@wochinge
Copy link
Contributor Author

wochinge commented Nov 3, 2020

the first version still technically breaks compatibility? I thought the idea is to allow users to train/run old 1.x bots without any changes to the files

In both cases we can check if the forms section is a list (old format) or a dict (new format) and set the flag / add the custom action for it on the fly.

2, do you mean that if there is an action with the same name, we override the default? I thought we already do it, at least we did for action_listen in some old version

Yep, that would be the default. Currently overriding the default form action however isn't possible due to our current check which we have in place.

@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

then I'd say number 2 is natural solution

@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

the only problem, users might start to create their custom forms, and they wouldn't work, because specific events wouldn't be returned

@wochinge
Copy link
Contributor Author

wochinge commented Nov 3, 2020

If they start to create custom forms, then they should really know what they are doing 😆

@wochinge wochinge mentioned this issue Nov 3, 2020
4 tasks
@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

If they start to create custom forms, then they should really know what they are doing 😆

we need to provide a safe guard mechanism. Our code should direct people who don't know what they are doing

@akelad
Copy link
Contributor

akelad commented Nov 3, 2020

i am still confused about what option 2 means practically

@wochinge
Copy link
Contributor Author

wochinge commented Nov 3, 2020

we need to provide a safe guard mechanism. Our code should direct people who don't know what they are doing

I don't agree: If they write their own versions of forms, then they need to know what they are doing. There is also no save guard mechanism if you override any other default action.

@akelad I implemented it in the linked PR. Does that clarify things?

@Ghostvv
Copy link
Contributor

Ghostvv commented Nov 3, 2020

If they write their own versions of forms, then they need to know what they are doing.

we cannot rely on that, we know that it is not the case. For example, people want to customize a form, they find out that they can override the action, and they do that instead of creating custom validate. We need to create intuitive software that requires minimum documentation if possible. It is not always possible, but we at least should try

There is also no save guard mechanism if you override any other default action.

in some sense, there is. There is no special return requirements for custom actions, so you cannot really destroy their promised performance

@wochinge
Copy link
Contributor Author

wochinge commented Nov 3, 2020

You could also override the FormAction within Rasa Open Source and we weren't worried about users misusing that. I'll definitely make it clear in the changelog / documentation that they should be careful when doing this.

@akelad
Copy link
Contributor

akelad commented Nov 4, 2020

ok i get the concept from the PR - but I don't understand what you mean by overriding the FormAction - how should they override it? imo the documentation should be clearer on how you should override it, and also state that it should only be used for the migration and no other uses. I think that's not clear right now

@wochinge
Copy link
Contributor Author

wochinge commented Nov 4, 2020

but I don't understand what you mean by overriding the FormAction - how should they override it?

Instead of executing the default action for a form which is now within Rasa Open Source it would send a call to the action server and execute whatever custom action users have there with the name of the form. Ideally this is the Rasa Open Source 1.x FormAction.

, and also state that it should only be used for the migration and no other uses. I think that's not clear right now

I think this part is quite explicit in my opinion as I repeated it in the changelog and the documentation with an additional caution section. What else would you suggest here?

@akelad
Copy link
Contributor

akelad commented Nov 4, 2020

You can override this default action with a custom action by
adding a custom action with the form's name to the domain. This can e.g. be used to use
the deprecated `FormAction` which is part of the Rasa SDK and used to implement forms
for Rasa Open Source 1.

"This can e.g. be used" - implies it's optional. I would say we don't need a caution section, but just specify then and there that this should only be done in the case of migration

@smsredojev
Copy link

Sorry, is there a currently a way to have a form with only slots that use custom slot mappings?

@wochinge
Copy link
Contributor Author

It is in fact possible as described in the docs. We have added support for this in this PR

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.

4 participants