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

Fixes #3939: vertexai gemini has a different URL, we should support that in the vertexai procedure #3947

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Feb 14, 2024

Fixes #3939

  • Now the URL can be configured via the apoc.conf apoc.ml.vertexai.url (like OpenAI procedures)

  • Added VertexAIHandler abstract factory to handle the body, jsonpath and URL based on the procedure

  • added apoc.ml.vertexai.stream, which calls gemini AI endpoints (which end with :streamGenerateContent instead of :predict)

  • added apoc.ml.vertexai.custom, similar to apoc.ml.bedrock.custom

    • we can customize the endpoint, model, resource (eg :streamGenerateContent or :predict)
  • updated documentation with configs and examples

  • added test cases with gemini-pro and gemini-pro-vision multimodal APIs: https://cloud.google.com/vertex-ai/docs/generative-ai/embeddings/get-multimodal-embeddings


Other considerations/changes

  • Some tests currently fail:
    • apoc.ml.VertexAIIT#chatCompletion, due to: class java.util.ArrayList cannot be cast to class java.util.Map
    • apoc.ml.VertexAIIT#completion, due to: assertEquals(true, result.containsKey("recitationResult"));

Most likely because the model is changed.
Hence, I generalized the assertions, as I did in this PR, by just asserting the text that should return.


For example, if we define no `endpoint` config.,
the default one `https://%1$s-aiplatform.googleapis.com/v1/projects/%2$s/locations/%1$s/publishers/google/models/%3$s:%4$s` will be valued, where:
- `%1$s` will be model with the `model` configuration
Copy link
Member

Choose a reason for hiding this comment

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

could we do this with named parameters instead, i.e.g {model} etc. will be easier for users to manage and handle.

Copy link
Member

@jexp jexp left a comment

Choose a reason for hiding this comment

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

could we do this with named parameters instead, i.e.g {model} etc. will be easier for users to manage and handle.

We can do a simple string replacement for these, nothing fancy.

…- updated image and assertion

The previoous image was sometimes recognized as a `bagel`,
and changed assertion from `tarallo` to `tarall` since the output can be both `tarallo` and `taralli`
Copy link
Member

@jexp jexp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much!

@vga91 vga91 merged commit fa7b262 into dev Feb 23, 2024
7 checks passed
@vga91 vga91 deleted the issue-3939 branch February 23, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants