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
AmazonQFeatureDev: Handle canned errors during code generation #4537
base: master
Are you sure you want to change the base?
Conversation
retryable: false, | ||
} | ||
} | ||
|
||
throw new ToolkitError('Code generation failed', { code: 'CodeGenFailed' }) |
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.
Won't we also want to know that code generation failed? I think this code will make it so the telemetry failure event never gets reported when we get a canned error
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.
This function is invoked from inside a amazonq_codeGenerationInvoke
telemetry block, so I assume we log the failure if we throw inside the block.
For the canned error case that we don't throw, we set
telemetry.setCodeGenerationResult(codegenResult.codeGenerationStatus.status)
so we will have the information of generations that failed.
@@ -411,9 +435,13 @@ export class FeatureDevController { | |||
let session | |||
try { | |||
session = await this.sessionStorage.getSession(message.tabID) | |||
if (session.state.codeGenerationResult?.result !== 'success') { | |||
throw new ToolkitError('Invalid state in code generation') |
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.
Is it worth extending a toolkiterror and defining a code just in case these cases are being hit and sending this data to telemetry? I see a couple places like these in the changes that might give us some potential insights if something is going very wrong on the users end
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.
Yeah good point! Those pieces of code should normally not be reachable, so it makes sense for us to get special telemetry on them.
I'll extend the ToolkitError for this.
@@ -144,7 +145,7 @@ export class FeatureDevController { | |||
}) | |||
} | |||
|
|||
private async processChatItemVotedMessage(tabId: string, messageId: string, vote: string) { | |||
private async processChatItemVotedMessage(tabId: string, _messageId: string, vote: string) { |
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.
probably just makes sense to remove it rather then underscore it if its not being used, but that can be a different PR
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.
question: Why so many model changes are needed for this handling?
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.
They're not all related to this change.
This json is auto generated, so this here is the latest version of the models.
@@ -300,6 +306,29 @@ export class FeatureDevController { | |||
this.messenger.sendAsyncEventProgress(tabID, false, undefined) | |||
} | |||
|
|||
private printFailedCodeGenMessage(session: Session, message: string, tabID: string, retryable: boolean) { |
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.
question: isn't this function missing the chat prompt blocking?
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.
The prompt is blocked on line 397. It's the last step of the function that calls this one.
this.messenger.sendChatInputEnabled(tabID, false) | ||
this.messenger.sendChatInputEnabled(tabID, enableInput) | ||
if (enableInput) { | ||
this.messenger.sendUpdatePlaceholder(tabID, 'How can this plan be improved?') |
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.
question: has this message been approved by johanna?
I think we need to synchronize to understand what message can be a follow up for retryable errors.
const deletedFiles = session.state.deletedFiles ?? [] | ||
|
||
if (session.state.codeGenerationResult?.result === 'pending') { | ||
throw new ToolkitError('Unexpected state in code generation') |
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.
question: shouldn't here be thrown InvalidCodeGenStateError?
if (session.state.codeGenerationResult?.result === 'failed') { | ||
// enable the input if the error is not retryable. | ||
// we're expecting a new prompt in this case | ||
enableInput = !session.state.codeGenerationResult.retryable |
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.
question: will there be failed code generation results that are non retryable?
Only canned errors are at the moment handled and they are all non retryable.
Problem
FeatureDev's code generation will sometimes return canned errors with detailed information about the problems that happened in the process. These errors are not retryable and should be worked around by the customer, using the information provided.
Solution
This CR adds the support for these canned errors, and adds dynamic logic for showing or hiding the
Retry
button whenever the code generation step fails.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.