-
Notifications
You must be signed in to change notification settings - Fork 480
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
AI Chat: pass selected model ID to back end #58338
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.
Nice!
const filteredModelIds = (settings.availableModelIds || []).filter(id => | ||
modelDescriptions.map(model => model.id).includes(id) | ||
); |
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.
non-blocking, but I think this'd be a little better with a some
call.
const filteredModelIds = (settings.availableModelIds || []).filter(id =>
modelDescriptions.some(model => model.id === id)
);
It's O(n^2) instead of O(n^3) (not that it really matters with such small loops), and I lean towards it being a little easier to read.
OTOH, I also acknowledge it's not that much easier to read, and maybe it loses something in legibility going to a some
call instead of a more clear sounding includes
call. 🤷
I dunno. Dealer's choice.
It could also be an external set:
const validModelIds = new Set(modelDescriptions.map(model => model.id));
const filteredModelIds = (settings.availableModelIds || []).filter(id => validModelIds.includes(id) );
That's clearer and faster (O(2n)), but it's also using an ugly external variable for just one thing.
Anyway, I'm just rambling.
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.
Ah yep, good call, I'll update this - might just go with some
since it's a relatively small set
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.
minor non-blocking suggestion, but otherwise lgtm 🚀
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.
🎉
@@ -2,7 +2,7 @@ | |||
<config><![CDATA[{ |
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.
Thanks for updating these!
Warning!!
The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.
Description
This hooks up the selected model ID in the front end to the endpoint we actually invoke on the back end, to target the desired model. I updated our JSON file with our three existing endpoints and some placeholder information, but we'll want to do a formal pass on the overview and training data for each.
Demo (just to showcase the ability to target different models - we're aware that the Mental Health Mistral model produces some less than ideal responses, but will address updating the available model options separately)
GenAIModelSelection.mp4
Links
https://codedotorg.atlassian.net/browse/LABS-669
Also @bencodeorg to your question in the JIRA, looks like we are logging the selected model ID to the DB already (screenshot of my local DB below):
Testing story
Tested with our existing endpoints.