-
Notifications
You must be signed in to change notification settings - Fork 401
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
FEAT: Add Conversation/Prompt ID information to exceptions (for MultiTurnOrchestrators) #637
base: main
Are you sure you want to change the base?
Conversation
@@ -83,7 +83,7 @@ async def send_prompt_async( | |||
) | |||
|
|||
await self._calc_hash_and_add_request_to_memory(request=error_response) | |||
raise | |||
raise Exception(f"Error sending prompt with conversation ID: {normalizer_request.conversation_id}") from ex |
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.
maybe a quick Null check just in case normalizer_request ends up as None so we don't re-throw in that case?
cid = normalizer_request.conversation_id if normalizer_request is not None else None
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.
NormalizerRequest should have a conversation ID of None anyways if it wasn't set (which it always is in the context of MTOs at least), so I'm thinking this Null check might not be needed? unless you were thinking of having a different Exception message if convID is None vs not None?
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.
Maybe! We just want this to be as resilient as we can because we're in a place where things have gone wrong. So my gut is that normalizer_request could be None, and then this code would re-trigger another exception and we wouldn't see the value
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.
looks good to me, just make sure you run pre-commit to fix those formatting issues! :)
This PR adds ID information to exceptions in two different places to aid understanding of which conversation ID or prompt ID caused the failure.
The first place is in PromptNormalizer, where conversation ID is exposed if an error is thrown when sending a prompt to a target. The second place is in Scorer within the
_score_value_with_llm
function. Again, if an exception is raised when sending the prompt (scorer llm request in this case) to the target, prompt ID of the prompt to score is exposed to the user.These two places were chosen because LLM prompt-sending interactions are the most likely to fail in a multi-turn conversation since LLMs act non-deterministically. Users can then use the IDs to more easily investigate the failure.
Testing: Two additional unit tests were added to ensure the correct Exception messages are raised in
PromptNormalizer.py
andScorer.py
.