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

Allow overriding a baseUri on GenerativeModel #109

Merged
merged 8 commits into from
Apr 4, 2024
Merged

Conversation

natebosch
Copy link
Collaborator

@natebosch natebosch commented Apr 3, 2024

Allow the Vertex AI SDK to be implemented on top of GenerativeModel
without exposing any change to end users. Add a side method to create a
GenerativeModel with a specific base URI. The model code and task will
be appended.

  • Add a createModelWithBaseUri top level method which is not exported
    outside of the src/ library.
  • Rename _baseUrl to _googleAiBaseUri to disambiguate against the
    _baseUri field. Use "uri" over "url" more consistently.

cynthiajoan and others added 5 commits March 28, 2024 09:06
Allow the Vertex AI SDK to be implemented on top of `GenerativeModel`
without exposing any change to end users. Add a side method to create a
`GenerativeModel` with a specific base URI. The model code and task will
be appended.

- Add a `createModelWithBaseUri` top level method which is not exported
  outside of the `src/` library.
- Rename `_baseUrl` to `_googleAiBaseUri` to disambiguate against the
  `_baseUri` field. Use "uri" over "url" more consistently.
- Remove the redundant Task enum name field in favor of the language
  level name getter.
This might come in conflict with being able to override the API version
in a new `RequestOptions` class.
@natebosch natebosch changed the title url overrides Allow overriding a baseUri on GenerativeModel Apr 3, 2024
@natebosch natebosch marked this pull request as ready for review April 3, 2024 22:16
@natebosch natebosch requested a review from devoncarew April 3, 2024 22:16
Copy link
Collaborator

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

👍 - this looks like a good solution.

I'm less familiar with the differences between the gemini and vertex protocols. If they are similar-but-different (slightly more possible return codes for block reasons? slightly different safety rating values?) we may have more work to do on the parsing side to make this SDK reusable. But allowing the vertex SDK to re-use much of the gemini one seems like a good goal.

@cynthiajoan
Copy link
Collaborator

👍 - this looks like a good solution.

I'm less familiar with the differences between the gemini and vertex protocols. If they are similar-but-different (slightly more possible return codes for block reasons? slightly different safety rating values?) we may have more work to do on the parsing side to make this SDK reusable. But allowing the vertex SDK to re-use much of the gemini one seems like a good goal.

Yeah that could be possible to be more diverge going forward, but for the current supported APIs the reuse is a more efficient approach. With the current setup we should be able to add logic if there starts to have more different behavior like response parsing, and when we reach the state if it's too complicated, we can replace the underneath implementation details without affecting Vertex SDK public apis.

@natebosch
Copy link
Collaborator Author

No changelog entry since this isn't user facing. I filed #111 to resolve this coupling point. I'll coordinate changes with @cynthiajoan around this method - I do think it may be necessary to change from a single Uri to a Uri Function(String apiVersion) since we will require overriding the API version for function calling.

@natebosch natebosch merged commit 53c83ad into main Apr 4, 2024
7 checks passed
@natebosch natebosch deleted the url-overrides branch April 4, 2024 20:19
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.

3 participants