-
-
Notifications
You must be signed in to change notification settings - Fork 57
[Platform] Use capabilities instead of instanceof checks #299
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
base: main
Are you sure you want to change the base?
[Platform] Use capabilities instead of instanceof checks #299
Conversation
…ring pre-request situations to reduce dependency on specific model class
…ace during pre-request situations to reduce dependency on specific model class
…e during response situations to reduce dependency on specific model class
Hey @JoshuaBehrens, I think you are on a good topic here, but we def need to align before we roll out changes like that - also looking at #300. I think as well that we'll need some changes on how the Platform works, and I was working on #136 to bring in a new mechanism for result conversion and reducing the repetitive implementations. And I def need to wrap that up before we gonna move on Capabilities/Actions/Tasks/etc. |
@chr-hertel fine to me :) I admit I expected it to start a discussion instead of an LGTM merge. I also assume, that we gonna need some model metadata to check before invoking it. Some instanceof check sort-of promised existence of the model which now is a lot of trust towards the user. And in ollama we already have some checks about embeddings and somewhere else is a listModels function. So there is also need for that :) Depending on the way "we need to align" I am not sure whether Github issue commentary is a helpful chat alternative |
@@ -35,7 +36,7 @@ public function __construct( | |||
|
|||
public function supports(Model $model): bool | |||
{ | |||
return $model instanceof Gemini; | |||
return $model->supports(Capability::INPUT_MESSAGES); |
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.
The way those model are used in the request method is
$url = \sprintf(
'https://generativelanguage.googleapis.com/v1beta/models/%s:%s',
$model->getName(),
$options['stream'] ?? false ? 'streamGenerateContent' : 'generateContent',
);
Which means that the name should be a valid model name for Gemini.
While it's kinda implicit when using a Gemini Model (and should be enforced by phpdoc), it's not the cade for a generic model...
So I dunno if you should have something like
return $model->supports(Capability::INPUT_MESSAGES)
&& \in_array($model->getName(), Gemini::MODELS, true)
Same for others supports methods.
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.
This is intended with my comment on
This eventually will reduce the amount of code you have to write to establish new providers that replicate existing API's with a well-known subset of models or add better support for custom models like OpenAI fine-tuned models or ollamas self-built models.
I am with you, that it would be great if you know in before whether the model exists but the API can tell with a 404. No need to update the library to support new models.
This commit bricks some code flows. Other changes needs to be done as well. See #300
In other issues and pull requests the model class should be used as a simple struct, only holding scalar values and should not have any business logic effect by inheritance. This has been the case for embeddings, completions and other API calls like Whisper audio, and Dall-e images. Right now this makes it difficult to share different instances of the model class with various providers, although a lot of providers share the same models, and therefore likely the same capabilities. This eventually will reduce the amount of code you have to write to establish new providers that replicate existing API's with a well-known subset of models or add better support for custom models like OpenAI fine-tuned models or ollamas self-built models.
To stop this, I have changed all instanceof checks matching a specific model subclass to refer to the expected input or output capabilities of the model, that were implemented in the specific messagenormalizer, resultconverter or modelclient. It was sometimes not easy to decide, which approach to go as not every input has a corresponding output capability. Some are merged behind a helpful name like tool calling or but when you are not familiar with the underlying decisions the name input_multiple is not easily relatable to a vector calculations done by embeddings features. So either this could need a refreshing rename, new capability or some documentation around it on the capability enum.