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

Allowing dictionary under slot_was_set in the stories schema #8142

Merged
merged 7 commits into from
Mar 10, 2021

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Mar 9, 2021

Proposed changes:

  • Updating the schema file stories.yml to allow the presence of a dictionary under slot_was_set key in the stories.yml.

Current investigation:

  • The validation of the yaml schemas happen in the function validate_yaml_schema at this point in the code where the validate function of the Core class (of pykwalify) is called.
  • Currently, the change I did was to update the stories schema to allow a dictionary under slow_was_set by adding the type map with allowempty : True which means that it can
    have keys which are not present in the schema, and these can map to anything.
  • I tried also type: "any" which also checks the stories.yml and returns it as a valid file but it feels like it undermines all the previous checks since based on the docs type any : will always be true no matter what the value is, even unimplemented types.
  • I was also thinking that maybe the ideal would be to check the stories schema file in combination with the domain schema file in order to do something like if the slot is of type any in the domain, it should pass the validation check in stories but I am not sure if this is possible and how in pykwalify. If it is possible then we could also take into account the ‘influence_conversation’ value in the domain file which can affect the check in the stories schema.

The solution in this PR is related and would be affected from the progress in this ticket.

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)

@Imod7 Imod7 requested a review from a team as a code owner March 9, 2021 05:13
@Imod7 Imod7 requested review from kalkbrennerei and removed request for a team March 9, 2021 05:13
@Imod7 Imod7 marked this pull request as draft March 9, 2021 05:13
@Imod7 Imod7 removed the request for review from kalkbrennerei March 9, 2021 05:14
@Imod7 Imod7 self-assigned this Mar 9, 2021
@Imod7 Imod7 linked an issue Mar 9, 2021 that may be closed by this pull request
@Imod7 Imod7 added the area:rasa-oss 🎡 Anything related to the open source Rasa framework label Mar 9, 2021
@Imod7 Imod7 requested a review from alwx March 9, 2021 05:45
@Imod7 Imod7 added the area:rasa-oss/training-data Issues focused around Rasa training data (stories, NLU, domain, etc.) label Mar 9, 2021
Copy link
Contributor

@alwx alwx left a comment

Choose a reason for hiding this comment

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

👍

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 10, 2021

Thank you so much for the review @alwx !

Two more things :

  • If you could you also take a look at the changelog I added that would be awesome! 🙏🏽
  • And also I wanted to ask you if I should add a test for this change or not? It looks like a small change to add a test but maybe I should since it affects the validation process.

@Imod7 Imod7 marked this pull request as ready for review March 10, 2021 13:14
@Imod7 Imod7 requested a review from alwx March 10, 2021 13:14
@@ -0,0 +1,10 @@
Allowing in `stories.yml` and under the key `slot_was_set` to have slots with values that result to a dictionary.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rephrase it a bit (e.g. "Allow to have slots with values that result to a dictionary in stories.yml and under the key slot_was_set") or at least replace "Allowing" with verb "Allow", but that's of course super minor and certainly up to you.

@Imod7 Imod7 enabled auto-merge March 10, 2021 17:00
@Imod7 Imod7 disabled auto-merge March 10, 2021 17:00
@Imod7 Imod7 enabled auto-merge March 10, 2021 17:00
@Imod7 Imod7 merged commit 79d9fc8 into main Mar 10, 2021
@Imod7 Imod7 deleted the rasa_train_slot_type_any_with_dict branch March 10, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/training-data Issues focused around Rasa training data (stories, NLU, domain, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rasa train fails when slot of type any contains a dictionary
2 participants