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

_utter_responses should handle template and response #8226

Closed
wants to merge 4 commits into from

Conversation

yennycheung
Copy link
Contributor

@yennycheung yennycheung commented Mar 17, 2021

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)
  • reformat files using black (please check Readme for instructions)

Manual testing
I tested this on formbot. Before, formbot wasn't able to respond with utter_wrong_num_people. After the change, it was able to.

@yennycheung yennycheung requested review from a team and wochinge and removed request for a team March 17, 2021 15:29
@yennycheung
Copy link
Contributor Author

@wochinge, I want to ask for your opinion here. I think we should still make the sdk change now, so that the deprecation message would make sense. Any dependencies I need to be aware of? If we push the sdk change first, it'll also make sense to update these https://github.com/RasaHQ/rasa/blob/main/examples/formbot/actions/actions.py#L51

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

I want to ask for your opinion here. I think we should still make the sdk change now, so that the deprecation message would make sense. Any dependencies I need to be aware of? If we push the sdk change first, it'll also make sense to update these

I agree somewhat. My reasoning behind doing Rasa Open Source first was that this bug blocks users right now while a weird deprecation message isn't blocking for users.

I'd make the SDK send both (template + response) keys for now to make the new SDK version keeps on working with older Rasa Open Source versions.

Depends all a bit on your time. I can do the SDK change tomorrow morning to support you?

rasa/core/actions/action.py Show resolved Hide resolved
@yennycheung
Copy link
Contributor Author

@wochinge, I looked into the SDK, but I got some questions, I'll ping you tomorrow. :)

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Looks great! I'd update the deprecation message to make it more actionable but great change otherwise.

if not generated_response:
generated_response = response.pop("template", None)
rasa.shared.utils.io.raise_deprecation_warning(
"The terminology 'template' is deprecated and replaced by 'response', use the `response` parameter instead of `template` in the dispatcher.",
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making this warning more actionable? E.g. upgrading to Rasa SDK xy or adapting their custom SDK?

rasa/core/actions/action.py Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

@yennycheung Can you please rebase this to be merged into 2.4.x? We need to cut a micro release with this change and this can only be done from 2.4.x

@yennycheung
Copy link
Contributor Author

Thanks @wochinge. I created a new PR pointing to 2.4.x at #8239

@wochinge wochinge closed this Mar 23, 2021
@wochinge wochinge deleted the bug_issue-8223 branch March 23, 2021 15:20
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.

dispatcher.utter_message(template=...) no longer works in a custom action
2 participants