Skip to content

Handle 429 from auto-model service#2474

Merged
charisk merged 2 commits intomainfrom
charisk/handle-llm-429
Jun 2, 2023
Merged

Handle 429 from auto-model service#2474
charisk merged 2 commits intomainfrom
charisk/handle-llm-429

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Jun 2, 2023

Shows a slightly more helpful message when we receive 429s from the auto-model endpoint.

Checklist

N/A - internal only:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested a review from a team as a code owner June 2, 2023 09:06
Comment thread extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts Outdated
@charisk charisk force-pushed the charisk/handle-llm-429 branch from 0b1aac1 to baf9818 Compare June 2, 2023 10:44
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, although the progress doesn't get cleared when I run into a rate limit error. I think you'll need to add await this.clearProgress(); just before return; in the generateModeledMethodsFromLlm method.

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Jun 2, 2023

LGTM, although the progress doesn't get cleared when I run into a rate limit error. I think you'll need to add await this.clearProgress(); just before return; in the generateModeledMethodsFromLlm method.

Ah, I was contemplating whether to tidy this up in this PR. I've added this.clearProgress() in the catch block to make sure we clear on other failures. too.

@koesie10
Copy link
Copy Markdown
Member

koesie10 commented Jun 2, 2023

Ah, I was contemplating whether to tidy this up in this PR. I've added this.clearProgress() in the catch block to make sure we clear on other failures. too.

Thanks! We might want to move that into a catch block inside the generateModeledMethodsFromLlm method to also catch errors when the query fails, but this is error is much more likely to happen, so that can happen in a follow-up.

@charisk charisk enabled auto-merge (squash) June 2, 2023 12:25
@charisk charisk merged commit 134c440 into main Jun 2, 2023
@charisk charisk deleted the charisk/handle-llm-429 branch June 2, 2023 12:51
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.

2 participants