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 loockup table validation #8168

Closed

Conversation

iurisevero
Copy link
Contributor

@iurisevero iurisevero commented Mar 10, 2021

Proposed changes:

  • Resolve issue Log warning if lookup tables are not used correctly #5956
  • Update the _add_lookup_table_regexes function, at (regex_featurizer.py)[https://github.com/RasaHQ/rasa/blob/1.10.x/rasa/nlu/featurizers/sparse_featurizer/regex_featurizer.py], to verify if there is some table in the training data.loockup_tables and return a bool informing it
  • Print a warning message if validation returns false

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)

rgstephens and others added 30 commits December 2, 2020 14:27
Support server list, drop group_id from producer
Backport Multiple Sanic Workers fix for 1.10.x
@sara-tagger
Copy link
Collaborator

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

@yennycheung
Copy link
Contributor

Thank you for your contribution @iurisevero. The solution looks great. Would you mind pointing the pull request to main instead of RasaHQ:1.10.x? Then we could include your changes in the next releases. Thank you!

@iurisevero
Copy link
Contributor Author

Hi @yennycheung, I'll change the pointing, no problem! But I don't think the CI will accept my changes, because I did them from the tag 1.10.23, once the issue was solved in 2.x versions but still happening at 1.10.x.

@iurisevero iurisevero changed the base branch from 1.10.x to main March 17, 2021 19:18
@yennycheung
Copy link
Contributor

Apologies for the late reply, it seems like there are a lot of unrelated changes, would you mind pulling the latest main and cherry-picking your changes against it?

@iurisevero
Copy link
Contributor Author

Hi. Don't worry. So, I don't think that pulling the latest main is a good idea. As I said, my changes were made for the 1.10.x version, where this issue still relevant. On the 2.x versions it was already solved.

My idea was to merge these changes to 1.10.x branch and fix this problem for the next 1.10 tag.
Does that make sense for you?

@yennycheung
Copy link
Contributor

@wochinge, do we do patches over the older versions or Rasa OSS?

@wochinge
Copy link
Contributor

We only do so if it's crucial. Rasa Open Source 2 is out since more than half a year and I'd highly encourage to switch to it. We unfortunately don't have the capacity to maintain two majors side by side at the moment.

@iurisevero
Copy link
Contributor Author

I get it. I believe this pull request will be closed so, right?
I suggest closing the issue #5956 as well. It doesn't make sense anymore.

@iurisevero iurisevero closed this Apr 27, 2021
@yennycheung
Copy link
Contributor

@iurisevero, thank you for your help on this one. Hope next time we can get your work merged! 🙏🏼

@ErickGiffoni ErickGiffoni deleted the 5956-log-warning-loockup-tables branch August 5, 2021 19:42
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.

10 participants