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

Cleanup deprecation warnings #6453

Merged
merged 25 commits into from
Aug 26, 2020
Merged

Cleanup deprecation warnings #6453

merged 25 commits into from
Aug 26, 2020

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Aug 19, 2020

Proposed changes:

  • cleanup some old deprecation warnings
  • new API to raise a deprecation warning, that requires a warn_until_version argument.
    (Original Slack thread)

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)

@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 19, 2020

@tmbo @erohmensing this is a second pass to remove deprecation warnings and introduce a new API to raise deprecation warnings. WDYT?

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Yes, this is what I had in mind!

rasa/core/tracker_store.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/formats/markdown.py Show resolved Hide resolved
rasa/utils/common.py Outdated Show resolved Hide resolved
@m-vdb m-vdb marked this pull request as ready for review August 21, 2020 09:53
@m-vdb m-vdb requested a review from wochinge August 21, 2020 12:45
@wochinge
Copy link
Contributor

@m-vdb I won't get to this before my vacation - can you please assign somebody else for review?

@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 21, 2020

sorry, completely forgot

@m-vdb m-vdb requested review from degiz and removed request for wochinge August 21, 2020 15:17
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

That's great! 🚀

One note about Removed support for string ``responses``(use dictionaries instead).:
Let's include that in the rasa/core/schemas/domain.yml, then it will be a part of our standard domain verification!

rasa/core/channels/mattermost.py Outdated Show resolved Hide resolved
@m-vdb m-vdb force-pushed the remove-deprecation-warnings branch from c6a1b9a to a487fa2 Compare August 25, 2020 07:31
rasa/core/schemas/domain.yml Outdated Show resolved Hide resolved
rasa/core/schemas/domain.yml Outdated Show resolved Hide resolved
@m-vdb m-vdb requested a review from degiz August 25, 2020 08:57
@m-vdb m-vdb force-pushed the remove-deprecation-warnings branch from dd4eb8f to d938060 Compare August 25, 2020 12:32
@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 25, 2020

@degiz I've implemented the changes we just talked about: keep nlu.yml the same as it doesn't need validation to go through; custom validation of "custom" and "text" keys inside of a response. Ready for review 🚀

rasa/core/schemas/domain.yml Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 25, 2020

@degiz last thing: test_nlg_fails_on_empty_response() is failing because of the changes I made in RasaYAMLReader. Should I remove the test? Why did we write it in the first place?

@degiz
Copy link
Contributor

degiz commented Aug 25, 2020

@m-vdb I've just noticed that responses are parsed as a part of NLU, it used to be a domain!

@tmbo since you've implemented the recent changes to response selector, we now have Domain::collect_templates() and RasaYAMLReader both operating on responses YAML key, was it done intentionally? Could it have be just one of the files?

@tmbo
Copy link
Member

tmbo commented Aug 25, 2020

yes and no - it could have been a single file, but previously we required users to move the responses into their own file which made it easier to implement this in the same way.

conceptually it should be possible to combine utter templates and response selector responses, we just need to make sure they end up in the right place

@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 26, 2020

@degiz from the commit on this file it looks like it was intended. The commit also introduced the changes to schemas/nlu.yml.

It looks to me that I should re-revert my changes:

  • create rasa/utils/schemas.yml file
  • define a partial schema for the responses key there
  • reference this partial schema in both domain.yml and nlu.yml

Do you agree?

@degiz
Copy link
Contributor

degiz commented Aug 26, 2020

@m-vdb I'm sorry things are so complicated 😄
Anyway, this "split responses" topic is definitely not part of this commit, so yes, please proceed with you original change.

@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 26, 2020

no problemo - good learning opportunity for me! Will push my changes

@m-vdb m-vdb requested a review from degiz August 26, 2020 08:48
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

🚀

@rasabot rasabot merged commit dbd3e94 into master Aug 26, 2020
@rasabot rasabot deleted the remove-deprecation-warnings branch August 26, 2020 10:08
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.

6 participants