-
Notifications
You must be signed in to change notification settings - Fork 6
Increasing timeouts for generation to 15 seconds #23
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
Conversation
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.
On an initial read through this seems like a hack. Instead of using the built in settings we're creating our own new settings value and manually overriding the defaults. we should instead leverage the default settings.
}, | ||
"providerTimeout": { | ||
"title": "Provider Timeout (milliseconds)", | ||
"description": "Timeout for completion provider requests in milliseconds. Recommended value is 15000 (15 seconds) for Qiskit Code Assistant API.", | ||
"type": "number", | ||
"default": 15000, | ||
"minimum": 1000, | ||
"maximum": 30000 | ||
}, | ||
"inlineProviderTimeout": { | ||
"title": "Inline Provider Timeout (milliseconds)", | ||
"description": "Timeout for inline completion provider requests in milliseconds.", | ||
"type": "number", | ||
"default": 15000, | ||
"minimum": 1000, | ||
"maximum": 30000 |
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.
These settings already exist in core jupyterlab. I'm unsure of the ramifications of duplicating them here
schema: ISettingRegistry.IProperty = { | ||
default: { | ||
enabled: true, | ||
timeout: 10000 |
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.
from what I can tell we can achieve the same results by just increasing this value instead of the surrounding code changes
get timeout(): number { | ||
return (this.settings.composite['providerTimeout'] as number) || 15000; | ||
} | ||
|
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.
(without checking the core code) is this overriding something? where is this called
Thanks for the review, Alex. I think this is not the proper way to do it, so I'll close it. |
Description
Increasing the
providerTimeout
andinlineProviderTimeout
to 15 seconds by defaultLinked Issue(s)
Fixes #22
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Manually
Checklist: