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

Form slot validation for 1st sentence #276

Merged
merged 5 commits into from Oct 5, 2020
Merged

Conversation

ArjaanBuijk
Copy link
Contributor

@ArjaanBuijk ArjaanBuijk commented Sep 23, 2020

Forms: slot validation for 1st sentence that triggered the intent:
The helper function Tracker.form_slots_to_validate returned an empty dictionary when there was no active_loop.

As a result, slot-validation was skipped for slots that were filled with entities extracted from the 1st sentence.

The fix is not to check on active_loop.

Fixes #269

Status (please check what you already did):

  • made PR ready for code review
  • updated the test test_get_extracted_slots_with_no_active_loop
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

This is needed to validate slots extracted from the 1st sentence that triggered the intent.
@CLAassistant
Copy link

CLAassistant commented Sep 23, 2020

CLA assistant check
All committers have signed the CLA.

This reverts commit 9e6174a.
@@ -241,9 +241,6 @@ def form_slots_to_validate(self) -> Dict[Text, Any]:

slots_to_validate = {}

if not self.active_loop:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this! I think we need another safe guard then that the slots were actually set by a form execution. We are creating the temporary tracker here , which is then sent to the action server.

One safeguard idea: If the action before the slots is not an ActionExecuted(form_name), then return an empty dict.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need the safe guard, because it searches for the past slots that were set before something else happened, if users want to access it, why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

then we should change the name of the function (this would also need to be changed in the docs). Up to @ArjaanBuijk imo

Copy link
Contributor

@Ghostvv Ghostvv Sep 29, 2020

Choose a reason for hiding this comment

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

+1 to renaming (we anyhow, get rid of form everywhere)

Copy link
Contributor

Choose a reason for hiding this comment

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

we anyhow, get rid of form everywhere)

Why? It is a form. No need to rename this because of loop

Copy link
Contributor

Choose a reason for hiding this comment

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

it can be used everywhere including loop, so it is not only for forms. I'd remove a guard and rename it to smth like last_set_slots

Copy link
Contributor

Choose a reason for hiding this comment

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

both options are fine to me (safe guard or renaming and removing safeguard) 👍

@wochinge
Copy link
Contributor

wochinge commented Oct 5, 2020

cc @alwx As this renames form_slots_to_validate to slots_to_validate

Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

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

🚀

@wochinge wochinge merged commit df94fb8 into master Oct 5, 2020
@wochinge wochinge deleted the form-slot-validation branch October 5, 2020 10:28
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.

tracker.form_slots_to_validate returns no slots on activation
4 participants