Alexandre/add base model option #276
Conversation
cohere/client.py
Outdated
|
|
||
| # Figuring out base model | ||
| if internal_custom_model_type in ["GENERATIVE", "CLASSIFICATION"]: | ||
| assert base_model is None, "base_model has to be None for generative and classification models" |
There was a problem hiding this comment.
this validation should happen on the backend
There was a problem hiding this comment.
Oh we had a discussion with Inna and Raphael, and we thought that it made more sense to do an internal mapping here like it's done for internal_custom_model_type with internal_custom_model_type = CUSTOM_MODEL_PRODUCT_MAPPING[model_type]
The other solution without the mapping would be to:
- use
base_model: str = "medium"as default argument of the function - this would force the user to specify
base_model="english"orbase_model="multilingual"for reranker.
I don't have a strong preference here. What do you think is the best solution?
There was a problem hiding this comment.
-
for non-rerank finetuning, we want to continue supporting
Noneas a value for the base_model. That allows us to change the default value on the backend. -
for rerank finetuning, the backend should throw an error "base_model cannot be empty for finetune type "rerank". Must be one of "english" or "multilingual"
no need to keep a map of defaults on the client. This allows us to control default values on the backend, so we can change them if needed without needing to ship a a new client and migrating users to it.
There was a problem hiding this comment.
For non-rerank finetuning, the base_model value has always been "medium":
cohere-python/cohere/client.py
Line 1120 in d21c04b
-> that's why I'm thinking of setting "medium" as the default value (to ensure no breaking change) if we don't keep a map of defaults on the client
There was a problem hiding this comment.
just like for all other apis, we should not be setting any defaults in the client, so lets just remove that "medium" setting from the SDK and make sure that the backend sets the correct value
There was a problem hiding this comment.
@raphaelcristal tagging you here as this is probably something you own. Do you think it's possible to do what @mkozakov is suggesting above? I.e. remove the "baseModel":"medium" default and instead add defaults to the backend?
There was a problem hiding this comment.
Yes what Michael says is correct. Best to keep all validation on the backend, so we don't have to deal with deprecations in the sdk. Sorry if I didn't explain this properly when we talked.
cohere/client.py
Outdated
| internal_base_model = "medium" | ||
| elif internal_custom_model_type == "RERANK": | ||
| internal_base_model = base_model or "english" | ||
| assert internal_base_model in [ |
cohere/client.py
Outdated
| assert base_model is None, "base_model has to be None for generative and classification models" | ||
| internal_base_model = "medium" | ||
| elif internal_custom_model_type == "RERANK": | ||
| internal_base_model = base_model or "english" |
There was a problem hiding this comment.
the default should be set on the backend
Add a new
base_modeloption for reranker finetuning to decide which base model to finetune from:englishormultilingual.For the other models (which don't have options), make sure that this variable is not filled.