Explicitly set "model" property for models that support it #783
Conversation
|
This feels really weird to make the model parsers do this instead of adding it as a centralized SDK functionality. See comments in here for how I think we should do it: #765 (comment) Do you want me to take over this task? I can do it |
Chatted offline with @rossdanlm, we are good to go with this change. Different model parsers can have different properties so it isn't possible to have a centralized place to do this. |
saqadri
force-pushed
the
pr783
branch
2 times, most recently
from
3424127 to
4925521
Compare
saqadri
requested review from
suyoglastmileai,
Ankush-lastmile,
jonathanlastmileai and
rossdanlm
as code owners
|
I think this is probably the right solution currently, just need to make sure it's well tested. Note that this takes the stance that an AIConfig can be valid even with a "dangling foreign key" to the global model mapping, and it's the parser's responsibility to fill in the gap. IMO it would be better to constrain AIConfigs to not have dangling pointers, but I don't know if that can actually be enforced currently via the CRUD API. |
saqadri
force-pushed
the
pr783
branch
3 times, most recently
from
ead4a6e to
4beeaf5
Compare
This change allows us to automatically set the "model" for the following model parsers: * OpenAI * Anyscale Endpoint * Hugging Face text generation * PaLM * Dall-E Without this change the local editor breaks because if a user doesn't explicitly specify the "model" property in settings metadata, inference fails for (some of) the above model parsers. This isn't ideal, because the user has already selected the model they want to use from the model selector, so shouldn't need to additionally specify a model id as well. There is still a larger issue of how we distinguish a model ID from a model parser ID. Filed #782 to come up with a proper fix for that.
|
Test plan: Validation.movAlso tested with dalle3 (not shown) |
Explicitly set "model" property for models that support it
This change allows us to automatically set the "model" for the following model parsers:
Without this change the local editor breaks because if a user doesn't explicitly specify the "model" property in settings metadata, inference fails for (some of) the above model parsers. This isn't ideal, because the user has already selected the model they want to use from the model selector, so shouldn't need to additionally specify a model id as well.
There is still a larger issue of how we distinguish a model ID from a model parser ID. Filed #782 to come up with a proper fix for that.