Custom and dynamic slots

[wochinge]

Proposed changes:

• fix Custom / dynamic slot mappings for forms rasa#7156
• users can now override a method required_slots to specify slots which should be filled by the form. The FormValidationAction will look for a method extract_<slot_name> to extract these slots.
• in case the users overrode required_slots, the FormValidationAction will automatically set the requested_slot to the next required_slot which is currently not filled. Users can override this behavior by customizing request_next_slot. By default request_next_slot also considers slots which were mapped in the domain (if there were any mapped ones).
• introduce a utility function utils.call_potential_coroutine to get rid of our checks if is coroutine ... else ...

I will update the Rasa Open Source documentation in a separate Rasa Open Source PR.

Status (please check what you already did):

• made PR ready for code review
• 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]

@samsucik I imagined something along these lines. What do you think?

[samsucik]

Looks good to me at a high level :-)

Would next_requested_slot() (in FormValidationAction) call required_slots() and attempt to select the next requested slot from the list that required_slots() returns?

[wochinge]

Would next_requested_slot() (in FormValidationAction) call required_slots() and attempt to select the next requested slot from the list that required_slots() returns?

I thought about it and decided that this would be something they should implement themselves. I want to avoid to change something for users who are just using this to validate their slots.

[samsucik]

That's fair. Though I think we should in some way communicate that the intended pattern (when one wants to change the required slots dynamically) is to call next_requested_slot() which then calls required_slots() where the logic for changing the set of required slots should be placed. What do you think about communicating this in the form of explanatory comments in the docstring of next_requested_slot() & required_slots() and possibly also some simple (commented out) code example in next_requested_slot()?

[samsucik]

Also, I think that with FormValidationAction it gets confusing that if your run() doesn't return any requested slots (and by the same logic if your next_requested_slot() & required_slots() don't return any slots to be filled), then your form will still continue if it's got some unfilled slots from the set defined in the domain. From looking at others' code but also from my own experience, I think that it's easy to fall in the trap of seeing your subclass of FormValidationAction as the only class responsible for the given form, without realising that there's one more layer above this class and that your subclass of FormValidationAction is just a way to interrupt/alter the behaviour provided by that above layer.

[wochinge]

Makes sense 👍 We can fill required_slots with the slot mappings from the domain by default and then always call request_next_slot on the next missing slot by default.

[samsucik]

Hmmm, this is an interesting idea. While it hides the "meta layer" even more, I think it also makes it very clear to the user how required_slots() and request_next_slot() can be used and how exactly they fit into the whole pattern. One risk I can see is that if the user doesn't explicitly set requested_slot to any value in run(), they might be very surprised to see that some next slot gets requested anyway (in case not all the slots -- as defined in the domain for the form -- are filled).

[wochinge]

How about we only return a requested_slot if required_slots was overridden?

[samsucik]

Sounds good to me! Together with an explanatory comment in run() (or also elsewhere) this should be clear enough I think.

[wochinge]

@samsucik Could you please have another look?

I split it up in custom slots and missing_slots. This way it's possible to implement a static version of custom_slots and still being able to request the next slot accordingly. It also allows for more precise overriding.

[akelad]

in which situations would you need to fill this out? you can specify slots without a slot mapping in the domain right?

[akelad]

ok i just realised why, in case you want to dynamically change the list of requested slots :D

[wochinge]

you'd override for two use cases:

• dynamic slots
• custom slot mappings

[akelad]

ok so the custom slot mapping part i'm confused about: if you specify your slots without a slot mapping in the domain file, shouldn't it just use the extract_ method for that slot?

[wochinge]

You mean that we would just try to extract every slot for which we have an extract_ function defined? I think that would work fine if the slot mappings are static. It would become tricky in case your required_slots change dynamically

[akelad]

ok i'm super confused now - the way to extract the slot changes? how would that even work?

[samsucik]

I don't think "my" users wanted to dynamically change how slots are extracted. They wanted to dynamically change the list of required slots, but extraction was either given right in the domain or implemented using extract_{slot} (in either case, the implementation didn't change). The way I understand it is that this new implementation should use extract_{slot} whenever it exists for the given slot, and use the "default" extraction approach otherwise.

[wochinge]

Sounds good for you @akelad ?

[akelad]

yes, that makes sense for me! So just to summarise, for the requested slot method that means you only need to fill it out if you want to dynamically change requested slots, correct?

[wochinge]

Exactly. This includes the case that you want to ignore / override slot mappings defined in the domain (ideally they just leave the slot mappings empty in the domain)

[akelad]

@wochinge anything in particular you want me to review? the example in the changelog looks good to me

[wochinge]

@akelad I mainly wanted your feedback on the overall approach and as you requested this feature multiple times.

[akelad]

looks great to me, aside from the discussion we're having in one of the comments now :)

[alwx]

Looks good!