feat: Add Gemini Embeddings #347
Conversation
valtzu
force-pushed
the
gemini-embeddings
branch
from
0ab04bb to
46306d7
Compare
|
Moved extra parameters to model's Also noticed that we probably need to parameterize But that'll of course be a separate PR. |
| public function __construct(string $name = self::GEMINI_EMBEDDING_EXP_03_07, array $options = []) | ||
| { | ||
| if (self::GEMINI_EMBEDDING_EXP_03_07 === $name) { | ||
| $options['dimensions'] ??= 1536; |
There was a problem hiding this comment.
I am not a big fan of rebuilding the api validation, if it changes we need to keep up.
What happens if you send a wrong dimensions value?
There was a problem hiding this comment.
This is not really about api validation, but about the fact that the Store implementations (at least MariaDB) have hardcoded dimensions currently and thus do not work with default settings.
This can probably be removed after MariaDB store will have dimensions in $options of initialize()
There was a problem hiding this comment.
Let's merge the options PR first then 👍🏻
There was a problem hiding this comment.
☝️ #348
I am not a big fan of rebuilding the api validation
Should I remove TaskType enum then also?
There was a problem hiding this comment.
To me the enum is ok, WDYT @chr-hertel ?
There was a problem hiding this comment.
Yea, i also Like the idea of providing that - nice for DX.
We only should take into account not to block users if there are new task types released by Google.
But the implementation still support plain string for that option instead an enum?
With huggingface i went with an interface:
https://github.com/php-llm/llm-chain/blob/main/src/Platform/Bridge/HuggingFace/Task.php instead
There was a problem hiding this comment.
Ok, thanks – I kept the enum around – and yes, string is accepted too so we're not restricting future model/parameter usage.
Also rebased after #348, I think this PR is good to go now 🚀
| public const EMBEDDING_001 = 'embedding-001'; | ||
|
|
||
| /** | ||
| * @param array{dimensions?: int, task_type?: TaskType|string, title?: string} $options |
There was a problem hiding this comment.
Do we use title somewhere? If not I would remove it from the array shape
There was a problem hiding this comment.
I would think it'd be useful for the user of this library, like in the example
There was a problem hiding this comment.
Ok I misread your comment, I thought it was about declaring params here with array shape in general
Yes, title is used, however, I think you are right that it does not belong to model options since a single model instance is probably intended for several documents
I'll take it from ModelClient::request()'s $options instead
tests/Platform/Bridge/Google/Embeddings/EmbeddingsModelClientTest.php
Outdated
Show resolved
Hide resolved
valtzu
force-pushed
the
gemini-embeddings
branch
2 times, most recently
from
614fbc1 to
a982687
Compare
valtzu
force-pushed
the
gemini-embeddings
branch
from
a982687 to
034da76
Compare
valtzu
force-pushed
the
gemini-embeddings
branch
from
034da76 to
e3eb6fc
Compare
This PR was merged into the main branch. Discussion ---------- feat: add Gemini Embeddings | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | | Issues | | License | MIT Cherry picking php-llm/llm-chain#347 > Add Embeddings support for Google Gemini. > > Only batch embedding implemented. > > Related to #28 Commits ------- 17e25d1 feat: add Gemini Embeddings (#347)
Add Embeddings support for Google Gemini.
Only batch embedding implemented.
Related to #28