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.

Sign up for GitHub

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

Jump to bottom

Response converter #7183

Merged
merged 26 commits into from
Nov 11, 2020
Merged

Response converter #7183

merged 26 commits into from
Nov 11, 2020

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Nov 4, 2020 •
edited
Loading

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)

@m-vdb m-vdb force-pushed the response-converter branch from 3273859 to 0d5f439 Compare November 5, 2020 16:38
@m-vdb m-vdb force-pushed the response-converter branch from 0d5f439 to feab7b6 Compare November 5, 2020 17:08
m-vdb
m-vdb commented Nov 5, 2020
View reviewed changes
rasa/cli/data.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 5, 2020

@degiz would you mind giving a first review on this PR? I haven't run nor written tests yet, I want to get some feedback on the general approach (no need to look at docstrings and function names for instance)

@m-vdb m-vdb requested a review from degiz November 5, 2020 17:11
degiz
degiz reviewed Nov 6, 2020
View reviewed changes
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.

Thanks a lot for working on this @m-vdb !!
One note about the general approach - can we use TrainingDataConverter interface? This way we can:

  • implement all needed conversions in another module and keep data.py from getting too long
  • reuse methods like TrainingDataConverter::generate_path_for_converted_training_data_file

rasa/cli/arguments/data.py Show resolved Hide resolved
rasa/cli/arguments/data.py Show resolved Hide resolved
rasa/cli/data.py Show resolved Hide resolved
rasa/cli/data.py Outdated Show resolved Hide resolved
rasa/cli/data.py Outdated Show resolved Hide resolved
@m-vdb m-vdb marked this pull request as ready for review November 6, 2020 16:12
@m-vdb m-vdb requested a review from degiz November 9, 2020 14:07
degiz
degiz reviewed Nov 10, 2020
View reviewed changes
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.

Thanks a lot for jumping in and implementing the changes!! 🚀 💯

One note - with this PR rasa data convert nlg will leave project in a broken state (domain and stories still not converted), could we extent the help text for this command and mention, that they should also run rasa data convert responses?

rasa/cli/data.py Outdated Show resolved Hide resolved
docs/docs/migration-guide.mdx Show resolved Hide resolved
docs/docs/migration-guide.mdx Outdated Show resolved Hide resolved
docs/docs/migration-guide.mdx Outdated Show resolved Hide resolved
rasa/cli/arguments/data.py Show resolved Hide resolved
rasa/nlu/training_data/converters/nlg_markdown_to_yaml_converter.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/converters/nlg_markdown_to_yaml_converter.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/converters/nlg_markdown_to_yaml_converter.py Outdated Show resolved Hide resolved
tests/cli/test_rasa_data.py Outdated Show resolved Hide resolved
rasa/cli/data.py Outdated Show resolved Hide resolved
m-vdb
m-vdb commented Nov 10, 2020
View reviewed changes
rasa/cli/data.py Outdated Show resolved Hide resolved
@m-vdb m-vdb requested a review from a team November 11, 2020 08:25
@m-vdb m-vdb requested a review from degiz November 11, 2020 09:27
degiz
degiz approved these changes Nov 11, 2020
View reviewed changes
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.

Thanks a lot! 🚀🚀🚀

Let's keep an eye on that domain dict key order 😬

rasa/shared/core/domain.py Outdated Show resolved Hide resolved
rasa/shared/core/domain.py
"""
# setting the `version` key first so that it appears at the top of YAML files
# thanks to the `should_preserve_key_order` argument of `dump_obj_as_yaml_to_string`
domain_data: Dict[Text, Any] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite surprised this even works 🤔 it's still a Dict, and the keys order is not preserved 😬
If tests fails, we could change the dict to the OrderedDict right away!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because they changed it in Python 3.7 \0/ => https://softwaremaniacs.org/blog/2020/02/05/dicts-ordered/

@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 11, 2020

thanks for the rapid reviews @degiz 🙏 🙏 🙏

@rasabot rasabot merged commit 970d6b4 into master Nov 11, 2020
@rasabot rasabot deleted the response-converter branch November 11, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joejuzl joejuzl joejuzl left review comments

@degiz degiz degiz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migration from old response selector format
4 participants
@m-vdb @joejuzl @degiz @rasabot