From 5a566a887ae9626bd7164f7b5cfed42471028b50 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Thu, 26 May 2022 11:44:35 +0100 Subject: [PATCH 1/6] fix extraction of slot with mapping conditions from intent that triggers the form --- rasa/core/actions/forms.py | 22 ++++++++++++++++++++++ rasa/core/actions/loops.py | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index dd88b68aa18d..a0e07a36e337 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -11,6 +11,7 @@ from rasa.core.actions.action import ActionExecutionRejection, RemoteAction from rasa.shared.core.constants import ( + ACTION_EXTRACT_SLOTS, ACTION_LISTEN_NAME, REQUESTED_SLOT, LOOP_INTERRUPTED, @@ -584,6 +585,27 @@ async def activate( # collect values of required slots filled before activation prefilled_slots = {} + # run slot extraction after activation + action_extract_slots = action.action_for_name_or_text( + ACTION_EXTRACT_SLOTS, domain, self.action_endpoint + ) + + logger.debug( + f"Executing default action '{ACTION_EXTRACT_SLOTS}' " f"at form activation." + ) + + extraction_events = await action_extract_slots.run( + output_channel, nlg, tracker, domain + ) + + events_as_str = "\n".join([str(e) for e in extraction_events]) + logger.debug( + f"Executed '{ACTION_EXTRACT_SLOTS}' resulted in " + f"these events: {events_as_str}." + ) + + tracker.update_with_events(extraction_events, domain) + for slot_name in self.required_slots(domain): if not self._should_request_slot(tracker, slot_name): prefilled_slots[slot_name] = tracker.get_slot(slot_name) diff --git a/rasa/core/actions/loops.py b/rasa/core/actions/loops.py index 4b9c50584fce..e11c73485bef 100644 --- a/rasa/core/actions/loops.py +++ b/rasa/core/actions/loops.py @@ -22,7 +22,7 @@ async def run( events = [] if not await self.is_activated(output_channel, nlg, tracker, domain): - events += self._default_activation_events() + tracker.update_with_events(self._default_activation_events(), domain) events += await self.activate(output_channel, nlg, tracker, domain) if not await self.is_done(output_channel, nlg, tracker, domain, events): From f106fa627bc79909b69b57e0d6274661b751d33d Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Thu, 26 May 2022 13:57:54 +0100 Subject: [PATCH 2/6] add fix for failing unit tests --- rasa/core/actions/forms.py | 18 ++++++++++++++++++ rasa/core/actions/loops.py | 20 ++++++++++++++++---- tests/core/actions/test_forms.py | 10 ++++++++-- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index a0e07a36e337..c82f4d4abe4b 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -678,3 +678,21 @@ async def deactivate(self, *args: Any, **kwargs: Any) -> List[Event]: """Deactivates form.""" logger.debug(f"Deactivating the form '{self.name()}'") return [] + + async def _activate_loop( + self, + output_channel: "OutputChannel", + nlg: "NaturalLanguageGenerator", + tracker: "DialogueStateTracker", + domain: "Domain", + events: List[Event], + ) -> List[Event]: + temp_tracker = tracker.copy() + + if not await self.is_activated(output_channel, nlg, tracker, domain): + default_activation_events = self._default_activation_events() + temp_tracker.update_with_events(default_activation_events, domain) + events += default_activation_events + events += await self.activate(output_channel, nlg, temp_tracker, domain) + + return events diff --git a/rasa/core/actions/loops.py b/rasa/core/actions/loops.py index e11c73485bef..bea474e9f94e 100644 --- a/rasa/core/actions/loops.py +++ b/rasa/core/actions/loops.py @@ -19,11 +19,9 @@ async def run( tracker: "DialogueStateTracker", domain: "Domain", ) -> List[Event]: - events = [] + events: List[Event] = [] - if not await self.is_activated(output_channel, nlg, tracker, domain): - tracker.update_with_events(self._default_activation_events(), domain) - events += await self.activate(output_channel, nlg, tracker, domain) + events = await self._activate_loop(output_channel, nlg, tracker, domain, events) if not await self.is_done(output_channel, nlg, tracker, domain, events): events += await self.do(output_channel, nlg, tracker, domain, events) @@ -92,3 +90,17 @@ async def deactivate( ) -> List[Event]: # can be overwritten return [] + + async def _activate_loop( + self, + output_channel: "OutputChannel", + nlg: "NaturalLanguageGenerator", + tracker: "DialogueStateTracker", + domain: "Domain", + events: List[Event], + ) -> List[Event]: + if not await self.is_activated(output_channel, nlg, tracker, domain): + events += self._default_activation_events() + events += await self.activate(output_channel, nlg, tracker, domain) + + return events diff --git a/tests/core/actions/test_forms.py b/tests/core/actions/test_forms.py index ee8714d15d90..463ae2400e61 100644 --- a/tests/core/actions/test_forms.py +++ b/tests/core/actions/test_forms.py @@ -47,6 +47,8 @@ async def test_activate(): mappings: - type: from_entity entity: number + conditions: + - active_loop: {form_name} forms: {form_name}: {REQUIRED_SLOTS_KEY}: @@ -90,10 +92,14 @@ async def test_activate_with_prefilled_slot(): mappings: - type: from_entity entity: {slot_name} + conditions: + - active_loop: {form_name} {next_slot_to_request}: type: text mappings: - type: from_text + conditions: + - active_loop: {form_name} """ domain = Domain.from_yaml(domain) events = await action.run( @@ -676,9 +682,9 @@ async def test_validate_slots_on_activation_with_other_action_after_user_utteran ) tracker.update_with_events(slot_events, domain) - action = FormAction(form_name, action_server) + form_action = FormAction(form_name, action_server) - events = await action.run( + events = await form_action.run( CollectingOutputChannel(), TemplatedNaturalLanguageGenerator(domain.responses), tracker, From 8291f17a6fa7f412f2374a105e1cb3e1b56f7638 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Thu, 26 May 2022 16:26:38 +0100 Subject: [PATCH 3/6] add unit test and make some adaptations --- rasa/core/actions/forms.py | 21 +++++++++-- tests/core/actions/test_forms.py | 65 ++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index c82f4d4abe4b..60f666bbed4d 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -585,13 +585,14 @@ async def activate( # collect values of required slots filled before activation prefilled_slots = {} - # run slot extraction after activation + # run slot extraction the second time during form activation + # for slots with mapping conditions action_extract_slots = action.action_for_name_or_text( ACTION_EXTRACT_SLOTS, domain, self.action_endpoint ) logger.debug( - f"Executing default action '{ACTION_EXTRACT_SLOTS}' " f"at form activation." + f"Executing default action '{ACTION_EXTRACT_SLOTS}' at form activation." ) extraction_events = await action_extract_slots.run( @@ -600,7 +601,7 @@ async def activate( events_as_str = "\n".join([str(e) for e in extraction_events]) logger.debug( - f"Executed '{ACTION_EXTRACT_SLOTS}' resulted in " + f"The execution of '{ACTION_EXTRACT_SLOTS}' resulted in " f"these events: {events_as_str}." ) @@ -615,10 +616,21 @@ async def activate( return [] logger.debug(f"Validating pre-filled required slots: {prefilled_slots}") - return await self.validate_slots( + + validate_events = await self.validate_slots( prefilled_slots, tracker, domain, output_channel, nlg ) + validated_slot_names = [ + event.key for event in validate_events if isinstance(event, SlotSet) + ] + + return validate_events + [ + event + for event in extraction_events + if isinstance(event, SlotSet) and event.key not in validated_slot_names + ] + async def do( self, output_channel: "OutputChannel", @@ -627,6 +639,7 @@ async def do( domain: "Domain", events_so_far: List[Event], ) -> List[Event]: + """Executes form loop after activation.""" events = await self._validate_if_required(tracker, domain, output_channel, nlg) if not self._user_rejected_manually(events): diff --git a/tests/core/actions/test_forms.py b/tests/core/actions/test_forms.py index 463ae2400e61..3820d0c5243b 100644 --- a/tests/core/actions/test_forms.py +++ b/tests/core/actions/test_forms.py @@ -1677,3 +1677,68 @@ async def test_form_slots_empty_with_restart(): tracker, domain, ) + + +async def test_extract_slots_with_mapping_conditions_during_form_activation(): + slot_name = "city" + entity_value = "London" + entity_name = "location" + + form_name = "test_form" + + domain = Domain.from_yaml( + f""" + entities: + - {entity_name} + slots: + {slot_name}: + type: text + mappings: + - type: from_entity + entity: {entity_name} + conditions: + - active_loop: {form_name} + forms: + {form_name}: + {REQUIRED_SLOTS_KEY}: + - {slot_name} + """ + ) + + events = [ + ActionExecuted("action_listen"), + UserUttered( + "I live in London", + entities=[{"entity": entity_name, "value": entity_value}], + ), + ] + tracker = DialogueStateTracker.from_events( + sender_id="test", evts=events, domain=domain, slots=domain.slots + ) + assert tracker.active_loop_name is None + + action_extract_slots = ActionExtractSlots(None) + events = await action_extract_slots.run( + CollectingOutputChannel(), + TemplatedNaturalLanguageGenerator(domain.responses), + tracker, + domain, + ) + assert events == [] + + expected_events = [ + ActiveLoop(form_name), + SlotSet(slot_name, entity_value), + SlotSet(REQUESTED_SLOT, None), + ActiveLoop(None), + ] + + form_action = FormAction(form_name, None) + form_events = await form_action.run( + CollectingOutputChannel(), + TemplatedNaturalLanguageGenerator(domain.responses), + tracker, + domain, + ) + + assert form_events == expected_events From 8c87ff1eac90aca80a73d27c3d56afbb8e9bae74 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Fri, 27 May 2022 10:34:57 +0100 Subject: [PATCH 4/6] add changelog, update docstring, duplicate tests for better coverage --- changelog/11149.bugfix.md | 1 + rasa/core/actions/forms.py | 9 ++-- tests/core/actions/test_forms.py | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 changelog/11149.bugfix.md diff --git a/changelog/11149.bugfix.md b/changelog/11149.bugfix.md new file mode 100644 index 000000000000..1d159dbad6f3 --- /dev/null +++ b/changelog/11149.bugfix.md @@ -0,0 +1 @@ +Fix the extraction of values for slots with mapping conditions from trigger intents that activate a form, which was possible in `2.x`. diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index 60f666bbed4d..38fb855df6bb 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -567,9 +567,10 @@ async def activate( ) -> List[Event]: """Activate form if the form is called for the first time. - If activating, validate any required slots that were filled before - form activation and return `Form` event with the name of the form, as well - as any `SlotSet` events from validation of pre-filled slots. + If activating, run action_extract_slots to fill slots with + mapping conditions from trigger intents. + Validate any required slots that can be filled, and return any `SlotSet` + events from the extraction and validation of these pre-filled slots. Args: output_channel: The output channel which can be used to send messages @@ -585,8 +586,6 @@ async def activate( # collect values of required slots filled before activation prefilled_slots = {} - # run slot extraction the second time during form activation - # for slots with mapping conditions action_extract_slots = action.action_for_name_or_text( ACTION_EXTRACT_SLOTS, domain, self.action_endpoint ) diff --git a/tests/core/actions/test_forms.py b/tests/core/actions/test_forms.py index 3820d0c5243b..d08e9dfa529e 100644 --- a/tests/core/actions/test_forms.py +++ b/tests/core/actions/test_forms.py @@ -41,6 +41,38 @@ async def test_activate(): action = FormAction(form_name, None) slot_name = "num_people" domain = f""" +slots: + {slot_name}: + type: float + mappings: + - type: from_entity + entity: number +forms: + {form_name}: + {REQUIRED_SLOTS_KEY}: + - {slot_name} +responses: + utter_ask_num_people: + - text: "How many people?" +""" + domain = Domain.from_yaml(domain) + + events = await action.run( + CollectingOutputChannel(), + TemplatedNaturalLanguageGenerator(domain.responses), + tracker, + domain, + ) + assert events[:-1] == [ActiveLoop(form_name), SlotSet(REQUESTED_SLOT, slot_name)] + assert isinstance(events[-1], BotUttered) + + +async def test_activate_with_mapping_conditions_slot(): + tracker = DialogueStateTracker.from_events(sender_id="bla", evts=[]) + form_name = "my form" + action = FormAction(form_name, None) + slot_name = "num_people" + domain = f""" slots: {slot_name}: type: float @@ -79,6 +111,47 @@ async def test_activate_with_prefilled_slot(): form_name = "my form" action = FormAction(form_name, None) + next_slot_to_request = "next slot to request" + domain = f""" + forms: + {form_name}: + {REQUIRED_SLOTS_KEY}: + - {slot_name} + - {next_slot_to_request} + slots: + {slot_name}: + type: any + mappings: + - type: from_entity + entity: {slot_name} + {next_slot_to_request}: + type: text + mappings: + - type: from_text + """ + domain = Domain.from_yaml(domain) + events = await action.run( + CollectingOutputChannel(), + TemplatedNaturalLanguageGenerator(domain.responses), + tracker, + domain, + ) + assert events == [ + ActiveLoop(form_name), + SlotSet(REQUESTED_SLOT, next_slot_to_request), + ] + + +async def test_activate_with_prefilled_slot_with_mapping_conditions(): + slot_name = "num_people" + slot_value = 5 + + tracker = DialogueStateTracker.from_events( + sender_id="bla", evts=[SlotSet(slot_name, slot_value)] + ) + form_name = "my form" + action = FormAction(form_name, None) + next_slot_to_request = "next slot to request" domain = f""" forms: From 0b6a58cd4fddd7123c807addfd51e99d9cb2b1df Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Fri, 27 May 2022 12:14:16 +0100 Subject: [PATCH 5/6] place back if statement in LoopAction.run --- rasa/core/actions/forms.py | 11 +++++------ rasa/core/actions/loops.py | 10 ++++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index 38fb855df6bb..715242600c5d 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -699,12 +699,11 @@ async def _activate_loop( domain: "Domain", events: List[Event], ) -> List[Event]: - temp_tracker = tracker.copy() + default_activation_events = self._default_activation_events() + events += default_activation_events - if not await self.is_activated(output_channel, nlg, tracker, domain): - default_activation_events = self._default_activation_events() - temp_tracker.update_with_events(default_activation_events, domain) - events += default_activation_events - events += await self.activate(output_channel, nlg, temp_tracker, domain) + temp_tracker = tracker.copy() + temp_tracker.update_with_events(default_activation_events, domain) + events += await self.activate(output_channel, nlg, temp_tracker, domain) return events diff --git a/rasa/core/actions/loops.py b/rasa/core/actions/loops.py index bea474e9f94e..afe128c9cbb9 100644 --- a/rasa/core/actions/loops.py +++ b/rasa/core/actions/loops.py @@ -21,7 +21,10 @@ async def run( ) -> List[Event]: events: List[Event] = [] - events = await self._activate_loop(output_channel, nlg, tracker, domain, events) + if not await self.is_activated(output_channel, nlg, tracker, domain): + events = await self._activate_loop( + output_channel, nlg, tracker, domain, events + ) if not await self.is_done(output_channel, nlg, tracker, domain, events): events += await self.do(output_channel, nlg, tracker, domain, events) @@ -99,8 +102,7 @@ async def _activate_loop( domain: "Domain", events: List[Event], ) -> List[Event]: - if not await self.is_activated(output_channel, nlg, tracker, domain): - events += self._default_activation_events() - events += await self.activate(output_channel, nlg, tracker, domain) + events += self._default_activation_events() + events += await self.activate(output_channel, nlg, tracker, domain) return events From fbe279cc616fed9887016b21a33f374da7f1519b Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Wed, 1 Jun 2022 12:33:03 +0100 Subject: [PATCH 6/6] apply review suggestions --- rasa/core/actions/forms.py | 14 +++---- rasa/core/actions/loops.py | 10 +++-- tests/core/actions/test_forms.py | 64 +++++++++++++++++--------------- 3 files changed, 46 insertions(+), 42 deletions(-) diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py index 715242600c5d..b301684e8ba4 100644 --- a/rasa/core/actions/forms.py +++ b/rasa/core/actions/forms.py @@ -598,7 +598,7 @@ async def activate( output_channel, nlg, tracker, domain ) - events_as_str = "\n".join([str(e) for e in extraction_events]) + events_as_str = "\n".join(str(e) for e in extraction_events) logger.debug( f"The execution of '{ACTION_EXTRACT_SLOTS}' resulted in " f"these events: {events_as_str}." @@ -616,15 +616,15 @@ async def activate( logger.debug(f"Validating pre-filled required slots: {prefilled_slots}") - validate_events = await self.validate_slots( + validated_events = await self.validate_slots( prefilled_slots, tracker, domain, output_channel, nlg ) validated_slot_names = [ - event.key for event in validate_events if isinstance(event, SlotSet) + event.key for event in validated_events if isinstance(event, SlotSet) ] - return validate_events + [ + return validated_events + [ event for event in extraction_events if isinstance(event, SlotSet) and event.key not in validated_slot_names @@ -697,13 +697,11 @@ async def _activate_loop( nlg: "NaturalLanguageGenerator", tracker: "DialogueStateTracker", domain: "Domain", - events: List[Event], ) -> List[Event]: - default_activation_events = self._default_activation_events() - events += default_activation_events + events = self._default_activation_events() temp_tracker = tracker.copy() - temp_tracker.update_with_events(default_activation_events, domain) + temp_tracker.update_with_events(events, domain) events += await self.activate(output_channel, nlg, temp_tracker, domain) return events diff --git a/rasa/core/actions/loops.py b/rasa/core/actions/loops.py index afe128c9cbb9..ccaa704a7cb0 100644 --- a/rasa/core/actions/loops.py +++ b/rasa/core/actions/loops.py @@ -22,8 +22,11 @@ async def run( events: List[Event] = [] if not await self.is_activated(output_channel, nlg, tracker, domain): - events = await self._activate_loop( - output_channel, nlg, tracker, domain, events + events += await self._activate_loop( + output_channel, + nlg, + tracker, + domain, ) if not await self.is_done(output_channel, nlg, tracker, domain, events): @@ -100,9 +103,8 @@ async def _activate_loop( nlg: "NaturalLanguageGenerator", tracker: "DialogueStateTracker", domain: "Domain", - events: List[Event], ) -> List[Event]: - events += self._default_activation_events() + events = self._default_activation_events() events += await self.activate(output_channel, nlg, tracker, domain) return events diff --git a/tests/core/actions/test_forms.py b/tests/core/actions/test_forms.py index d08e9dfa529e..a353f4c7eba7 100644 --- a/tests/core/actions/test_forms.py +++ b/tests/core/actions/test_forms.py @@ -40,21 +40,23 @@ async def test_activate(): form_name = "my form" action = FormAction(form_name, None) slot_name = "num_people" - domain = f""" -slots: - {slot_name}: - type: float - mappings: - - type: from_entity - entity: number -forms: - {form_name}: - {REQUIRED_SLOTS_KEY}: + domain = textwrap.dedent( + f""" + slots: + {slot_name}: + type: float + mappings: + - type: from_entity + entity: number + forms: + {form_name}: + {REQUIRED_SLOTS_KEY}: - {slot_name} -responses: - utter_ask_num_people: - - text: "How many people?" -""" + responses: + utter_ask_num_people: + - text: "How many people?" + """ + ) domain = Domain.from_yaml(domain) events = await action.run( @@ -72,23 +74,25 @@ async def test_activate_with_mapping_conditions_slot(): form_name = "my form" action = FormAction(form_name, None) slot_name = "num_people" - domain = f""" -slots: - {slot_name}: - type: float - mappings: - - type: from_entity - entity: number - conditions: - - active_loop: {form_name} -forms: - {form_name}: - {REQUIRED_SLOTS_KEY}: + domain = textwrap.dedent( + f""" + slots: + {slot_name}: + type: float + mappings: + - type: from_entity + entity: number + conditions: + - active_loop: {form_name} + forms: + {form_name}: + {REQUIRED_SLOTS_KEY}: - {slot_name} -responses: - utter_ask_num_people: - - text: "How many people?" -""" + responses: + utter_ask_num_people: + - text: "How many people?" + """ + ) domain = Domain.from_yaml(domain) events = await action.run(