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

Change missing domain log level in rasa-sdk from ERROR to DEBUG #1113

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Change missing domain log level in rasa-sdk from ERROR to DEBUG #1113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4b765a
Change missing domain log level in rasa-sdk from ERROR to DEBUG
aleksandarmijat Jun 26, 2024
6785065
Add changelog, update log message
aleksandarmijat Jun 27, 2024
8686cfe
Merge branch 'main' into ATO-2564-Change-missing-domain-log-level-in-…
aleksandarmijat Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rasa_sdk/endpoint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ async def webhook(request: Request) -> HTTPResponse:
body = {"error": e.message, "action_name": e.action_name}
return response.json(body, status=404)
except ActionMissingDomainException as e:
logger.error(e)
logger.debug(e)
body = {"error": e.message, "action_name": e.action_name}
return response.json(body, status=449)

Expand Down
4 changes: 3 additions & 1 deletion rasa_sdk/interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Copy link
Member

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?

Suggested change
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."

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

)

def __str__(self) -> Text:
return self.message
Loading