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

from_trigger_intent slot mapping with an active_loop condition fills the slot only when that form gets activated #11588

Conversation

ancalita
Copy link
Member

@ancalita ancalita commented Sep 20, 2022

Proposed changes:

  • Fix for ATO-334, reported issue that slot with a from_trigger_intent mapping with an active_loop condition was being filled, despite that particular form not being activated.

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)

@ancalita ancalita changed the title refine from_trigger mapping check from_trigger_intent slot mapping with an active_loop condition fills the slot only when that form gets activated Sep 20, 2022
@github-actions
Copy link
Contributor

🚀 A preview of the docs have been deployed at the following URL: https://11588--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@ancalita ancalita marked this pull request as ready for review September 21, 2022 12:23
@ancalita ancalita requested a review from a team as a code owner September 21, 2022 12:23
@ancalita ancalita requested review from znat and tmbo and removed request for a team September 21, 2022 12:23
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ancalita ancalita merged commit 8ace235 into 3.2.x Sep 26, 2022
@ancalita ancalita deleted the ATO-334-from-trigger-intent-slot-does-not-take-into-account-active-loop-condition branch September 26, 2022 15:37
Copy link
Contributor

@znat znat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just wondering if we can have an easier to read condition when deciding to fill slots with FROM_TRIGGER_INTENT mapping

Comment on lines +1286 to +1293
if tracker.active_loop_name is None:
trigger_mapping_condition_met = False
elif (
active_loops_in_mapping_conditions
and tracker.active_loop_name is not None
and (tracker.active_loop_name not in active_loops_in_mapping_conditions)
):
trigger_mapping_condition_met = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be simplified to a single if statement? Something like:

if (tracker.active_loop_name is None 
    or active_loops_in_mapping_conditions
    and tracker.active_loop_name 
    and (tracker.active_loop_name not in active_loops_in_mapping_conditions)
):
       trigger_mapping_condition_met = False

IIUC we want to make sure that:

  • There's an active loop
    AND either/or:
  • a condition states that we should only fill for the current active loop
  • no condition is set for any other form

May be something like this would work then:

should_fill_trigger_slot = (
  mapping_type == SlotMappingType.FROM_TRIGGER_INTENT
  and tracker.active_loop_name 
  and (tracker.active_loop_name in active_loops_in_mapping_conditions if active_loops_in_mapping_conditions else True)
    )

form_slots
and slot.name not in form_slots
and mapping_type != SlotMappingType.FROM_TRIGGER_INTENT
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this warning irrelevant when mapping FROM_TRIGGER_INTENT?

Copy link
Member Author

@ancalita ancalita Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is irrelevant, because it's not necessary to add the slot with FROM_TRIGGER_INTENT mapping to the form's required_slots (since it gets filled automatically at form activation without it being requested by the form) and at the same time we want to allow a FROM_TRIGGER_INTENT filled slot to specify in which form context they should be filled via a mapping condition.

While this validation check is meant for any other slot mapping type that has mapping conditions specified and should be part of the form's required_slots, it's not applicable for this FROM_TRIGGER_INTENT mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants