Form switching #7771

ArjaanBuijk · 2021-01-21T21:44:19Z

Proposed changes:
Fixes for two issues when switching forms:

Core re-asks for slot upon form interruption #7751 : Core re-asks for slot upon form interruption
Form switching fails when requested_slot is the same in both forms #7710 : Form switching fails when requested_slot is the same in both forms

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)

Fix for issue #7751 to avoid that a form will re-ask for the slot prior to switching to another form.

Fix for issue #7710

wochinge

Core re-asks for slot upon form interruption

In my opinion this can't happen due to

rasa/rasa/core/actions/forms.py

Line 492 in cbed574

if await self.is_done(output_channel, nlg, tracker, domain, events_so_far):

, no?

forms is first called for the original form. - The slot validation is called on the sentence that will trigger the form switching, but the original form is still active. The slot validation will fail and will also return ActionExecutionRejected event - Need to avoid raising the ActionExecutionRejected event too soon, because other slots might be set by the custom action. Reverted that change with a comment to clarify. - Need to avoid asking for the requested slot by checking if ActionExecutionRejected event was returned. The form is not yet done, so the is_done() method is still False.

ArjaanBuijk · 2021-01-22T19:30:35Z

@wochinge ,

Core re-asks for slot upon form interruption

In my opinion this can't happen due to

rasa/rasa/core/actions/forms.py

Line 492 in cbed574

if await self.is_done(output_channel, nlg, tracker, domain, events_so_far):

, no?

It actually does happen, because the loop is first done with the original form still active.

The sentence that will trigger the next form is first validated for the requested slot of the orginal form
The validation will fail, but the form is not done, and it will ask for the next slot if we don't prevent this

I restored the logic to avoid raising the ActionExecutionRejected to quickly, so other slots set by the custom validation action will be set, but add an if statement that avoids the form asking for the next slot if ActionExecutionRejected was returned.

wochinge

Can you please add a changelog entry?
I suggest merging to 2.2.x as it's a bug fix
FYI: @nbeuchat is currently fixing similar parts with valide slots when coming back from unhappy paths #7764

rasa/core/actions/forms.py

Co-authored-by: Tobias Wochinger <[email protected]>

wochinge · 2021-01-27T10:21:51Z

Just re-request review once you you're done 🙌🏻

changelog/7751.bugfix.md

rasa/core/actions/forms.py

tests/core/actions/test_forms.py

Co-authored-by: Tobias Wochinger <[email protected]>

tests/core/actions/test_forms.py

- simpler call to PolicyPrediction, using all defaults - Build up an `events_expected` list as we go, and assert it against the tracker - Include the `BotUttered()` event in the `events_expected` list that is asserted.

ArjaanBuijk · 2021-02-05T16:36:38Z

@wochinge ,
I like the way the test for form switching is looking after implementing your suggestions. Let me know what you think.

wochinge

Looks good 🚀 Nice work! Do you want to target 2.2.x so you can release a micro with the fix? Alternatively we're also releasing a minor on Thursday.

ArjaanBuijk · 2021-02-08T15:48:07Z

Looks good Nice work! Do you want to target 2.2.x so you can release a micro with the fix? Alternatively we're also releasing a minor on Thursday.

@wochinge ,
I prefer to release in the minor on Thursday.

wochinge

🚀

wochinge · 2021-02-08T16:58:00Z

Nice work @ArjaanBuijk ! 💯

ArjaanBuijk added 2 commits January 21, 2021 15:16

Do the ActionExecutionRejection independent if user rejected manually

7de47da

Fix for issue #7751 to avoid that a form will re-ask for the slot prior to switching to another form.

Ignore REQUESTED_SLOT when switching to another form.

ef115ed

Fix for issue #7710

ArjaanBuijk requested review from wochinge, Ghostvv and desmarchris January 21, 2021 21:44

wochinge reviewed Jan 22, 2021

View reviewed changes

Base automatically changed from master to main January 22, 2021 11:15

ArjaanBuijk added 2 commits January 22, 2021 16:03

When ActionExecutionRejected, the form no longer asks for the next slot

a48889b

Forms will only fill the requested slot if it is active

8161219

ArjaanBuijk requested a review from wochinge January 22, 2021 21:42

wochinge reviewed Jan 25, 2021

View reviewed changes

ArjaanBuijk and others added 5 commits January 26, 2021 14:01

Make it a protected method.

1ee2ebd

Co-authored-by: Tobias Wochinger <[email protected]>

Update docstring

24aff50

Co-authored-by: Tobias Wochinger <[email protected]>

Update docstring

322fe0b

Co-authored-by: Tobias Wochinger <[email protected]>

Update docstring

efcfcd7

Co-authored-by: Tobias Wochinger <[email protected]>

Update docstring & use the protected method

fc67440

ArjaanBuijk added 3 commits January 27, 2021 11:38

Fix lint-docstrings

c9b91dd

Add test for switching forms when first slot is same in both forms

b3fb312

Add changelogs

3253ab0

ArjaanBuijk requested review from wochinge and removed request for Ghostvv and desmarchris January 27, 2021 17:16

ArjaanBuijk added 2 commits January 27, 2021 12:18

Merge branch 'main' into form-switching

c6370de

Merge branch 'main' into form-switching

a5f767a

wochinge reviewed Feb 2, 2021

View reviewed changes

ArjaanBuijk and others added 2 commits February 2, 2021 11:15

Update rasa/core/actions/forms.py

ec43b8d

Co-authored-by: Tobias Wochinger <[email protected]>

No need for async

aa95b56

Co-authored-by: Tobias Wochinger <[email protected]>

ArjaanBuijk and others added 9 commits February 2, 2021 11:22

no longer async

ae39b1e

Co-authored-by: Tobias Wochinger <[email protected]>

Update tests/core/actions/test_forms.py

ab5c9a9

Co-authored-by: Tobias Wochinger <[email protected]>

Update tests/core/actions/test_forms.py

dde0081

Co-authored-by: Tobias Wochinger <[email protected]>

Use full-blown docstring with Args & Returns for _user_rejected_manually

f34c2ce

Merge remote-tracking branch 'origin/form-switching' into form-switching

dafda77

No longer async

45577b9

Rename text_xa into utter_ask_form_x

cdefacb

test_switch_forms_with_same_slot now uses MessageProcessor

fda20b4

Merge branch 'main' into form-switching

015e36a

ArjaanBuijk requested a review from wochinge February 2, 2021 21:20

wochinge reviewed Feb 3, 2021

View reviewed changes

TyDunn added this to the 2.3 Rasa Open Source milestone Feb 5, 2021

Update switching forms test

d723704

- simpler call to PolicyPrediction, using all defaults - Build up an `events_expected` list as we go, and assert it against the tracker - Include the `BotUttered()` event in the `events_expected` list that is asserted.

ArjaanBuijk requested a review from wochinge February 5, 2021 16:36

Merge branch 'main' into form-switching

cade65a

wochinge approved these changes Feb 8, 2021

View reviewed changes

Merge branch 'main' into form-switching

1d292f4

wochinge approved these changes Feb 8, 2021

View reviewed changes

ArjaanBuijk merged commit 5195217 into main Feb 8, 2021

ArjaanBuijk deleted the form-switching branch February 8, 2021 16:49

wochinge mentioned this pull request Apr 19, 2021

Last utterance is filling slot in new form #7712

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form switching #7771

Form switching #7771

ArjaanBuijk commented Jan 21, 2021 •

edited

Loading

wochinge left a comment

ArjaanBuijk commented Jan 22, 2021

wochinge left a comment

wochinge commented Jan 27, 2021

ArjaanBuijk commented Feb 5, 2021

wochinge left a comment

ArjaanBuijk commented Feb 8, 2021

wochinge left a comment

wochinge commented Feb 8, 2021

Form switching #7771

Form switching #7771

Conversation

ArjaanBuijk commented Jan 21, 2021 • edited Loading

wochinge left a comment

Choose a reason for hiding this comment

ArjaanBuijk commented Jan 22, 2021

wochinge left a comment

Choose a reason for hiding this comment

wochinge commented Jan 27, 2021

ArjaanBuijk commented Feb 5, 2021

wochinge left a comment

Choose a reason for hiding this comment

ArjaanBuijk commented Feb 8, 2021

wochinge left a comment

Choose a reason for hiding this comment

wochinge commented Feb 8, 2021

ArjaanBuijk commented Jan 21, 2021 •

edited

Loading