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

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

Conversation

aleksandarmijat
Copy link
Contributor

Proposed changes:

  • Updated the log message from error to debug
  • Rephrased the message so it's less alarming and actions are more clear

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:

Screenshot 2024-06-26 at 11 57 09

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

Copy link
Member

@ancalita ancalita left a 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?

@@ -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.

@aleksandarmijat aleksandarmijat requested a review from ancalita June 27, 2024 11:05
@aleksandarmijat aleksandarmijat merged commit 8c41dea into main Jun 27, 2024
16 checks passed
@aleksandarmijat aleksandarmijat deleted the ATO-2564-Change-missing-domain-log-level-in-rasa-sdk-from-ERROR-to-DEBUG branch June 27, 2024 13:02
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.

2 participants