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

Add EntityExtractor as a requirement for EntitySynonymMapper #6236

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Add EntityExtractor as a requirement for EntitySynonymMapper #6236

merged 2 commits into from
Jul 22, 2020

Conversation

AMR-KELEG
Copy link
Contributor

@AMR-KELEG AMR-KELEG commented Jul 17, 2020

Closes #6198

Proposed changes:

  • Add EntityExtractor as a requirement for EntitySynonymMapper in a pipeline

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)

Comment:
I failed to test the PR locally (I have missed up with my clone and I haven't been able to build Rasa from source yet).
I would really appreciate if you can test the edit.
The change is intuitive yet it needs to be tested.

@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @akelad will take a look at it as soon as possible ✨

@sara-tagger sara-tagger requested a review from akelad July 20, 2020 06:00
@akelad
Copy link
Contributor

akelad commented Jul 20, 2020

@tabergma do you know why that wasn't there in the first place?

@tabergma
Copy link
Contributor

@akelad No idea. We changed the way how to define those required components a couple of month ago. It might be that is was not possible beforehand and we forgot to add it once we changed. But this is definitely a good requirement that we should add.

@akelad
Copy link
Contributor

akelad commented Jul 20, 2020

agreed, was just checking whether there was a reason we omitted it 👌

Copy link
Contributor

@akelad akelad left a comment

Choose a reason for hiding this comment

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

thanks for the quick fix @AMR-KELEG! looks good + also works as intended.

Could you add a changelog entry about this bug fix as well please?

@AMR-KELEG
Copy link
Contributor Author

AMR-KELEG commented Jul 20, 2020

Thanks @akelad for verifying that the change works.
I will try to figure out how to properly build Rasa from source soon.
I have added an entry in the changelog as requested.

On the other hand, Should also the doc rst file be modified?

@akelad
Copy link
Contributor

akelad commented Jul 21, 2020

good find, yes that should also be changed there!

Copy link
Contributor

@akelad akelad left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this great contribution!!

@akelad akelad merged commit 26791a9 into RasaHQ:master Jul 22, 2020
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.

No warning when EntitySynonymMapper is positioned incorrectly in config.yml
4 participants