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

Making rasa data convert config update config.yml and data/rules.yml correctly with Mapping Policy #7984

Merged
merged 10 commits into from
Mar 3, 2021

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Feb 17, 2021

Proposed changes in Mapping Policy & Model Config Migration:

  • When migrating the MappingPolicy we append the RulePolicy in config.yml even if new_rules are empty.
  • When migrating the model config, we create the rules.yml file even if it is empty.
  • Added the test case of having only one policy (the Mapping Policy) and no rules.

Status (please check what you already did):

  • added a test case for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

- When migrating the mapping policy we append the RulePolicy in config.yml even if new_rules are empty.
- When migrating the model config, we create the rules.yml file even if it is empty.
@Imod7 Imod7 linked an issue Feb 17, 2021 that may be closed by this pull request
@Imod7 Imod7 marked this pull request as draft February 17, 2021 18:30
Imod7 added 3 commits February 19, 2021 16:51
- Changed the test expected output to include RulePolicy.
- Adjusted the success message at the end of the migration.
- Changed the criteria (to write in rules and config yaml file) to check also if policies list is not empty.
- In test `test_migrate_mapping_policy_to_rules` added the case of having only one policy (the Mapping Policy) and no rules.
- Small fix in the code quality
- Added a changelog
@Imod7 Imod7 requested a review from joejuzl February 22, 2021 15:03
@Imod7
Copy link
Contributor Author

Imod7 commented Feb 23, 2021

Hey @alwx while checking the CI warnings, I bumped into this one :

UserWarning: Found a rule-based policy in your pipeline but no rule-based training data. Please add rule-based stories to your training data or remove the rule-based policy (RulePolicy) from your your pipeline.

So based on this warning I should not create a rules.yml file (if there are no rules available) neither append the RulePolicy in the config file. I guess the only thing that I should do is remove the MappingPolicy from the config file during migration and change the success message. What do you think?

@Imod7 Imod7 requested review from alwx and removed request for joejuzl February 24, 2021 10:24
rasa/cli/data.py Show resolved Hide resolved
rasa/cli/data.py Show resolved Hide resolved
rasa/cli/data.py Outdated Show resolved Hide resolved
rasa/core/config.py Show resolved Hide resolved
Copy link
Contributor Author

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

So I kept this logic, to create during the MappingPolicy migration an empty rules.yml file and add a RulePolicy in the config.yml file even if it is not necessarily used.

But I also left the warning check (and the related test) as it is right now. So we will still have the following warnings raised :

  • If there is a RulePolicy and no rule data, it raises a warning that we should add rule data.
  • If there are rule data and no RulePolicy, it raises a warning that we should add the policy.

I think it is important to leave these warnings (I am actually referring to the first one) since it is something that the user should take note of.

Also, the success message now changes to the following :

  • if there is a MappingPolicy and no rules, the message is
    "The migration generated 0 rules so no rules were added to 'data/rules.yml'."

  • if there is a MappingPolicy and at least one rule, the message is
    "The migration generated 1 rule which was added to 'data/rules.yml'."

rasa/cli/data.py Show resolved Hide resolved
@Imod7 Imod7 removed the request for review from alwx March 1, 2021 08:46
@Imod7 Imod7 requested review from koaning and joejuzl March 1, 2021 08:52
@Imod7 Imod7 marked this pull request as ready for review March 3, 2021 14:43
@Imod7 Imod7 removed the request for review from koaning March 3, 2021 16:01
@Imod7 Imod7 enabled auto-merge March 3, 2021 18:16
@Imod7 Imod7 self-assigned this Mar 3, 2021
@Imod7 Imod7 merged commit e3e4991 into main Mar 3, 2021
@Imod7 Imod7 deleted the rasa_data_convert_config branch March 3, 2021 18:50
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.

rasa data convert config does not create data/rules.ymlfile
3 participants