Skip to content

Conversation

tverney
Copy link
Contributor

@tverney tverney commented Jul 9, 2024

Problem

Currently we don't handle specific Guardrails type exception for code generation status detail, which leads to throw a general error and not help user self diagnose the prompt.

Solution

This PR proposes:

  • Specific error handling for GuardrailsException, PromptRefusalException, EmptyPatchException, ThrottlingException
  • Small refactor to use CodeGenerationStatus enum instead of single string

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

case CodeGenerationStatus.DEBATE_FAILED:
case CodeGenerationStatus.FAILED: {
switch (true) {
case codegenResult.codeGenerationStatusDetail?.includes('Guardrails'): {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the source of codeGenerationStatusDetail? is it an exception caught by mynah, or LSP, or some other component? If it's an exception, does it have a code that can be checked instead of string-matching the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our BE its returning the exception in this format:

reason: ${error_type.slice(0, error_type.indexOf('Exception'))}

Which leads to a combination of exception plus the error code like this:

Mocked error message reason: Guardrails

So answering your question, don't have a code because the identifier comes with the message itself so requires string-match those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't the backend model the errors with a code? Surely this is possible with smithy/coral? Do you have plans to change this?

This is resulting in a lot of low-value boilerplate code in all of your clients (such as this repo).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we've had a brief discussion about this internally. BE team said that we couldn't afford an API shape model change at this point because would require an extra bar raising process. So, for now, as far I've understood from their response, it was decided to leverage the existing details field mapping the exception code within the message. Those strings are extracted from the exception code, the only difference is that its combined with the details string, which is a details message coming from the BE which wasn't being filled until now.

Copy link

Choose a reason for hiding this comment

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

Yes we have plans to improve the API shape for error propagation. In Q4 there is a planned UX/UI revamp work of our service and we plan to improve the backend models as part of that effort. This is more of a short term improvement to reduce some pain points that our customers have encountered.

@tverney tverney requested a review from justinmk3 July 10, 2024 17:06
@tverney tverney requested a review from a team as a code owner July 11, 2024 13:50
@tverney tverney requested a review from jeffpyj July 11, 2024 20:42
@tverney tverney requested a review from hayemaxi July 15, 2024 22:56
@nkomonen-amazon nkomonen-amazon merged commit 469fa91 into aws:master Jul 16, 2024
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.

7 participants