Skip to content
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

add: google vertex ai embedding function #91

Merged

Conversation

iwilltry42
Copy link
Contributor

Copy link
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfect, I already wanted to add Vertex, Bedrock and Voyage AI! 👍

WDYT about using Vertex or GCP in the var/struct/function names instead of Google? Then Amazon would be Bedrock or AWS.

And regarding the options, WDYT about using the builder pattern (e.g. last section in https://rednafi.com/go/dysfunctional_options_pattern/)?

@iwilltry42
Copy link
Contributor Author

@philippgille I renamed the provider to vertex and switched to using the builder pattern as you suggested - I have no strong preference there.
One nit I have is that now the signature looks quite different from the other providers, so we may want to adjust them as well at some point in a breaking change. WDYT?

iwilltry42 added a commit to gptscript-ai/knowledge that referenced this pull request Jul 24, 2024
## Features

- new flag `-c`/`--config-file` to define a config file
- config file that currently only supports setting an embeddings provider (see [examples/configfiles/embedding_provider.yaml](https://github.com/gptscript-ai/knowledge/pull/44/files#diff-bf3c118e3a3433a2e694f0735d980f439d44fdbd87a1fd58cfcfa32536d0dbc1))
- embeddings providers:
	- Cohere
	- Google Vertex AI ([PR for chromem-go](philippgille/chromem-go#91))
	- OpenAI (+ Azure)
	- Cohere
	- Vertex
	- Jina
	- Mistral
	- Mixedbread
	- LocalAI
	- Ollama

## Breaking Changes

- removes all OpenAI flags -> now only available via env and config file
@iwilltry42 iwilltry42 requested a review from philippgille July 24, 2024 15:54
@philippgille
Copy link
Owner

philippgille commented Jul 28, 2024

@philippgille I renamed the provider to vertex and switched to using the builder pattern as you suggested - I have no strong preference there.
One nit I have is that now the signature looks quite different from the other providers, so we may want to adjust them as well at some point in a breaking change. WDYT?

Oh you changed it already ⚡ , sorry I just wanted to hear your thoughts first.

One way to not change the existing embedding func constructor signatures is to only use the builder pattern for the optional options struct that you created initially. So keeping the mandatory config (like API key) as individual params in the NewEmbeddingFunc...() signature, and adding a config struct for the optional configs as last param. That config struct then has the With...() methods.

That gives us:

  • The existing constructors that only have mandatory options can stay the same
  • Fewer global package members (no chromem.With...() function for each optional option)
  • Subjectively simpler due to passing a struct instead of variadic functions
  • No risk of naming conflicts with options for other embedding providers. (For example grpc-go has one WithChainUnaryInterceptor() to create a ClientOption and ChainUnaryInterceptor() (without the With prefix) to create a ServerOption, making it inconsistent) Not a risk with the naming you used, e.g. WithVertex...()

And the parameter for the optional config can be a pointer, so people can pass nil, in which case we use the default options. So people don't have to call call two constructor functions.

The one downside of that approach is that for this latter case (default config for the optional options) you still have to explicitly pass nil, instead of omitting the param like the variadic functions allow.


To not put the burden on you, I'll merge this PR and create another PR with my suggestion.

Thanks again!

@philippgille philippgille merged commit 55f437c into philippgille:main Jul 28, 2024
6 checks passed
@philippgille
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants