-
Notifications
You must be signed in to change notification settings - Fork 522
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: proactively show code generation iterations #5282
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type": "Feature", | ||
"description": "Amazon Q/dev proactively show code generation iterations" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,13 +254,18 @@ abstract class CodeGenBase { | |
newFiles: NewFileInfo[] | ||
deletedFiles: DeletedFileInfo[] | ||
references: CodeReference[] | ||
codeGenerationRemainingIterationCount?: number | ||
codeGenerationTotalIterationCount?: number | ||
}> { | ||
for ( | ||
let pollingIteration = 0; | ||
pollingIteration < this.pollCount && !this.tokenSource.token.isCancellationRequested; | ||
++pollingIteration | ||
) { | ||
const codegenResult = await this.config.proxyClient.getCodeGeneration(this.conversationId, codeGenerationId) | ||
const codeGenerationRemainingIterationCount = codegenResult.codeGenerationRemainingIterationCount | ||
const codeGenerationTotalIterationCount = codegenResult.codeGenerationTotalIterationCount | ||
|
||
getLogger().debug(`Codegen response: %O`, codegenResult) | ||
telemetry.setCodeGenerationResult(codegenResult.codeGenerationStatus.status) | ||
switch (codegenResult.codeGenerationStatus.status) { | ||
|
@@ -273,6 +278,8 @@ abstract class CodeGenBase { | |
newFiles: newFileInfo, | ||
deletedFiles: getDeletedFileInfos(deletedFiles, workspaceFolders), | ||
references, | ||
codeGenerationRemainingIterationCount: codeGenerationRemainingIterationCount, | ||
codeGenerationTotalIterationCount: codeGenerationTotalIterationCount, | ||
} | ||
} | ||
case 'predict-ready': | ||
|
@@ -321,7 +328,9 @@ export class CodeGenState extends CodeGenBase implements SessionState { | |
public deletedFiles: DeletedFileInfo[], | ||
public references: CodeReference[], | ||
tabID: string, | ||
private currentIteration: number | ||
private currentIteration: number, | ||
public codeGenerationRemainingIterationCount?: number, | ||
public codeGenerationTotalIterationCount?: number | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these fields optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, we do need them to be optional, since at the very first time when we asking for code generation, we don't know this iteration counts. We could have some default value passed when we initiate the CodeGenState, but that might misrepresent the meaning of "remaining/total counts". Hence I would prefer to leave it optional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, this is because this is getting CodeGenResult instead of CodeGenStatus. Sounds good |
||
) { | ||
super(config, tabID) | ||
} | ||
|
@@ -358,6 +367,9 @@ export class CodeGenState extends CodeGenBase implements SessionState { | |
this.filePaths = codeGeneration.newFiles | ||
this.deletedFiles = codeGeneration.deletedFiles | ||
this.references = codeGeneration.references | ||
this.codeGenerationRemainingIterationCount = codeGeneration.codeGenerationRemainingIterationCount | ||
this.codeGenerationTotalIterationCount = codeGeneration.codeGenerationTotalIterationCount | ||
|
||
action.telemetry.setAmazonqNumberOfReferences(this.references.length) | ||
action.telemetry.recordUserCodeGenerationTelemetry(span, this.conversationId) | ||
const nextState = new PrepareCodeGenState( | ||
|
@@ -367,7 +379,9 @@ export class CodeGenState extends CodeGenBase implements SessionState { | |
this.deletedFiles, | ||
this.references, | ||
this.tabID, | ||
this.currentIteration + 1 | ||
this.currentIteration + 1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious: there is any risk that currentIteration be different from those counts? And if its, have we tested what is shown to the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally the currentIteration would be simply "totaliteration-remainingIteration". This is calculated in the front end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so do we need remove currentIteration since we now have new counts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this field |
||
this.codeGenerationRemainingIterationCount, | ||
this.codeGenerationTotalIterationCount | ||
) | ||
return { | ||
nextState, | ||
|
@@ -480,7 +494,9 @@ export class PrepareCodeGenState implements SessionState { | |
public deletedFiles: DeletedFileInfo[], | ||
public references: CodeReference[], | ||
public tabID: string, | ||
private currentIteration: number | ||
private currentIteration: number, | ||
public codeGenerationRemainingIterationCount?: number, | ||
public codeGenerationTotalIterationCount?: number | ||
) { | ||
this.tokenSource = new vscode.CancellationTokenSource() | ||
this.uploadId = config.uploadId | ||
|
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.
Consider putting public fields before private fields
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.
We couldn't because "A required parameter cannot follow an optional parameter."
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.
Sounds good, let's follow the linter