-
Notifications
You must be signed in to change notification settings - Fork 234
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
Change missing domain log level in rasa-sdk from ERROR to DEBUG #1113
Change missing domain log level in rasa-sdk from ERROR to DEBUG #1113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 💯 could you pls add a changelog misc entry too?
rasa_sdk/interfaces.py
Outdated
@@ -386,7 +386,9 @@ class ActionMissingDomainException(Exception): | |||
|
|||
def __init__(self, action_name: Text, message: Optional[Text] = None) -> None: | |||
self.action_name = action_name | |||
self.message = message or "Domain context is missing." | |||
self.message = ( | |||
message or "Missing domain context, assistant will retry the request." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a brief user-facing explanation why domain might be missing, maybe even a link to the docs where this would be explained?
message or "Missing domain context, assistant will retry the request." | |
message or "Missing domain context, assistant will retry the request and include the domain into the request payload." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do it like this initially, but thought I should make it more concise and shorter. I'm up for having a more detailed message. I'll edit it.
Regarding the link to the docs, you mean this PR should wait for the docs to be ready so it could be linked here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the link to the docs, you mean this PR should wait for the docs to be ready so it could be linked here?
You can already add a link if you know the section where the docs will go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it will be included here because we're describing the request like this:
When your assistant predicts a custom action, the Rasa server sends a POST request to the action server with a json payload including the name of the predicted action, the conversation ID, the contents of the tracker and the contents of the domain.
We will edit the last part of the sentence that mentions the domain being included in the payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, although I'd add an extensive paragraph to describe both how the assistant operates now and the motivation and performance advantages for this new approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll go with this link, but if we decide we want to have a dedicated page just for this behavior that would be on another link, I'll make sure to update this.
…rasa-sdk-from-ERROR-to-DEBUG
Proposed changes:
You can see here, when
--debug
is used, debug logs make a lot of sense. Request is received, domain context is missing, request is received again, running action is finished:Status (please check what you already did):
black
(please check Readme for instructions)