839 optionally return chat model in start controller and level controller #858
Conversation
pmarsh-scottlogic
requested review from
chriswilty,
dhinrichs-scottlogic and
AAloshine-scottlogic
and removed request for
chriswilty
| function setChatModelId(modelId: string) { | ||
| if (!chatModel) { | ||
| console.error( | ||
| 'You are trying to change the id of the chatModel but it has not been loaded yet' |
There was a problem hiding this comment.
Not convinced this error message will help a future developer work out what's going on here. Can you come up with something indicating we only expect chat model to be changed on Sandbox level?
Or if this genuinely can happen, then I don't think we want to be rendering the dropdown model selector until we have the model response, but I was under the impression that chatModel and "available models" now arrive in the same response.
There was a problem hiding this comment.
"available models" arrives under the startResponse, along with chatModel. So if the game is loaded up or refreshed on sandbox, then the available models and chatModel arrive at the same time. However if the user switches from any level to sandbox, then there will be some amount of time (usually negligible) where the chat model dropdown and choose button are rendered, but the chatModel is not yet loaded.
There was a problem hiding this comment.
Okay, conditionally rendering the select model bits depending on chatModel being loaded. Wondering if we're doing this then we should also not render the model configuration sliders...
There was a problem hiding this comment.
"available models" arrives under the startResponse, along with chatModel. So if the game is loaded up or refreshed on sandbox, then the available models and chatModel arrive at the same time. However if the user switches from any level to sandbox, then there will be some amount of time (usually negligible) where the chat model dropdown and choose button are rendered, but the chatModel is not yet loaded.
Right, that makes sense now 😅
I'd say if we're going down this route, then we should conditionally render the sliders as you suggest. Can you see how that looks - in the code I mean, check it's not too gnarly?
There was a problem hiding this comment.
As discussed in the standup, we'll leave this for a future issue, as it has some (re)design implications.
pmarsh-scottlogic
deleted the
839-optionally-return-chatModel-in-startController-and-levelController
branch
…ller (#858) * return chatModel in level and start controllers * removes getModel endpoint * frontend start and leveServices now get the chatModel from the response * save chatModel in MainComponent * chatModel is drilled down to the components that need it * now sets the chatModelId so that the message updates * change a null to an undefined for brevity * remove old comment * remove getModel method from chatService * changes chatModel in LevelGetRequest to be optional * conditionally render the chat model selection * move default configs out of function * wraps comment line * improve config update method
⚠NOTE: I've put the wrong ticket number on the branch name.
Description
The final PR for #824 !
This one is to stop the getModel calls. We now return the current configured model in getStart or getLevel (but only for sandbox).
Screenshots
Before
Refreshing on sandbox (preview mode, so only single render):
changing level to sandbox (preview mode, so only single render):
After
Refreshing on sandbox (preview mode, so only single render):
changing level to sandbox (preview mode, so only single render):
Notes
Here's the whole situation because it's a bit convoluted:
chatModel. This records the id of the model that we want to chat to as well as the configurable settings (temperature, top p, etc). But we use the defaultchatModelrather than this configured version for all levels apart from sandbox. So really what this value represents is thechatModelused in sandbox (despite the name and it not being part of thelevelState). I think this is confusing, but I tried to change it and it snowballed into a bit of a mess, so 'm calling it a problem for another day.StartResponseandLoadLevelResponse. But because this is only relevant for sandbox, on all other levels this value is undefined.MainComponentwe consume these response objects. We set thechatModeluseState value, and then drill that down all the way to theModelSelectionandModelConfigurationcomponents, which show up in the control panel in sandbox.chatModelwith the backend's one, i.e. it will only store the configured chatModel used in the sandbox.Concerns
chatModelacross the front and back end to something likesandboxChatModeljust to make it really clear what it's there for.Checklist
Have you done the following?