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

fix: handling validation errors in approach/code generation #5245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: handling validation errors in approach/code generation #5245

wants to merge 1 commit into from

Conversation

gasolima
Copy link

@gasolima gasolima commented Jun 28, 2024 •
edited
Loading

Problem

When a validation errors(repo size exceeds limit or corrupted zip file) thrown from approach/code generation it will show generic internal error to the user

Solution

Catch those errors and show proper errors

Screenshot 2024-06-28 at 08 44 13
Screenshot 2024-06-28 at 08 42 34

REMINDER:
- Read CONTRIBUTING.md first.
- Add test coverage for your changes.
- Update the changelog using `npm run newChange`.
- Link to related issues/commits.
- Testing: how did you test your changes?
- Screenshots (if the pull request is related to UI/UX then please include light and dark theme screenshots)

-->

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
kumsmrit
kumsmrit reviewed Jun 28, 2024
View reviewed changes
packages/core/src/amazonqFeatureDev/controllers/chat/controller.ts
this.messenger.sendErrorMessage(
'',
message.tabID,
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it good to not provide retry for this, as this conversation will be counted ?
Copy link
Author

Choose a reason for hiding this comment

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

As many retries the user will do will not help, since this only can happen when our toolkit has a bug, then we need to have another version of the toolkit
kumsmrit
kumsmrit reviewed Jun 28, 2024
View reviewed changes
packages/core/src/amazonqFeatureDev/constants.ts Outdated
@@ -15,7 +15,7 @@ export const featureDevChat = 'featureDevChat'
export const featureName = 'Amazon Q Developer Agent for software development'

// Max allowed size for file collection
export const maxRepoSizeBytes = 200 * 1024 * 1024
export const maxRepoSizeBytes = 250 * 1024 * 1024
Copy link
Contributor

@kumsmrit kumsmrit Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we changing this ? The current public documentation says 200MB limit
Copy link
Author

Choose a reason for hiding this comment

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

This shouldn't be committed, removing it from the CR
kumsmrit
kumsmrit reviewed Jun 28, 2024
View reviewed changes
packages/core/src/amazonqFeatureDev/client/featureDev.ts Outdated
@@ -230,6 +238,18 @@ export class FeatureDevClient {
e.code === 'ServiceQuotaExceededException')
) {
throw new CodeIterationLimitError()
} else if (
isAwsError(e) &&
e.name === 'ValidationException' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

For CodewhispererRuntimeException, we will have to use e.code for verifying exception type.
@kumsmrit kumsmrit mentioned this pull request Jun 28, 2024
Merged
5 tasks
jpinkney-aws
jpinkney-aws reviewed Jun 28, 2024
View reviewed changes
packages/core/src/amazonqFeatureDev/client/featureDev.ts Outdated
@@ -184,6 +185,13 @@ export class FeatureDevClient {
e.name === 'ServiceQuotaExceededException'
) {
throw new PlanIterationLimitError()
} else if (
e.name === 'ValidationException' &&
e.message.includes('repo size is exceeding the limits')
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this same message is being used twice in two different places. Can we extract it somewhere?

Same with zipped file is corrupted
Copy link
Author

Choose a reason for hiding this comment

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

sure good idea
@gasolima gasolima force-pushed the add-validation-exceptions branch 2 times, most recently from de7e1de to 06fc70a Compare June 28, 2024 14:32
@gasser-mamdouh
fix: handling validation errors in approach/code generation
d12165e 
Problem: When a validation errors(repo size exceeds limit or corrupted zip file) thrown from approach/code generation it will show generic internal error to the user
Solution: Catch those errors and show proper errors
@justinmk3
Copy link
Contributor

CI failing. Also needs a rebase.
@justinmk3 justinmk3 added the needs-response Waiting on reply from issue/PR author. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-response Waiting on reply from issue/PR author.
5 participants
@gasolima @justinmk3 @jpinkney-aws @kumsmrit @gasser-mamdouh