-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
add API to get a list of all available models #1160
Conversation
Test Code:
Sample output:
|
Hi, thanks for the contribution. This unfortunately doesn't address the issue properly - that's maybe on us because the issue you linked to doesn't have any description. I'll add further detail on monday. This should be requesting model information from the backend services in the AI module. I'll try to remember to add more information about this by the end of Monday. |
@KernelDeimos Sounds good, waiting for further clarification will try to work on it in the meantime and guess around what's needed. |
Hi again, this request to backend gets all supported models await puter.drivers.call('puter-chat-completion', 'ai-chat', 'models'); This endpoint doesn't split it into "models" and "providers", but it does provide enough information for you to derive that. |
@KernelDeimos so you want to retrieve the dict returned by calling this and then process that dict to create the dict that I hardcoded? |
Yes that's correct |
@avijh squashed our commits into one since this is a small PR. |
src/puter-js/src/modules/AI.js
Outdated
providers = new Set(); // Use a Set to store unique providers | ||
|
||
models.result.forEach((item) => { | ||
if (item.provider) { |
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.
if (item.provider) { | |
if (item.provider) { |
src/puter-js/src/modules/AI.js
Outdated
if (item.provider) { | ||
providers.add(item.provider); | ||
} |
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.
I think this is supposed to be intended one more level
if (item.provider) { | |
providers.add(item.provider); | |
} | |
if (item.provider) { | |
providers.add(item.provider); | |
} |
src/puter-js/src/modules/AI.js
Outdated
if (item.provider && item.id) { | ||
if (provider) { | ||
if (item.provider === provider) { // Only add models for the specified provider | ||
if (!modelsByProvider[item.provider]) { |
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.
Whenever you see if if if if
, you can be sure there's a simpler approach. This amount of nesting increases a metric in static code analysis known as "cognitive complexity" which is a measure of how difficult the code is for somebody to understand and maintain later.
We can use guard clauses (also I found this video which is a bit long but you can get the jist of it by just scanning through real quick and looking at how he's changing the code) to simplify (untested):
...forEach(item => {
if ( ! item.id ) return;
if ( provider && ( item.provider !== provider ) ) return;
if ( ! modelsByProvider[item.provider] ) {
modelsByProvider[item.provider] = [];
}
modelsByProvider[item.provider].push(item.id);
}
Technically we could even combine the first two if
statements, but I think it's easier to read when separate concerns still belong on separate lines.
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.
Agreed. I modified the code per your suggestions and also removed redundant {} for readability.
Manual test passed. I did manual testing early since I know testing this without API keys is difficult. I'll do a quick test again after my review comments are addressed, and then this will be ready to merge. pr-1160_1.7x.mp4 |
Nice work! Thanks for addressing my comments |
Addressing #828
It adds two calls listModels() and listProviders().