Skip to content

Conversation

@Jan-Kazlouski-elastic
Copy link
Contributor

@Jan-Kazlouski-elastic Jan-Kazlouski-elastic commented Oct 6, 2025

This PR adds changes to specification caused by elastic/elasticsearch#134080
Additional actions

  • Signed the CLA
  • Executed make contrib

@Jan-Kazlouski-elastic Jan-Kazlouski-elastic self-assigned this Oct 6, 2025
@Jan-Kazlouski-elastic Jan-Kazlouski-elastic added the skip-backport This pull request should not be backported label Oct 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2025

Following you can find the validation changes against the target branch for the API.

API Status Request Response
search 🔴 2588/2615 → 2590/2615 2615/2615

You can validate this API yourself by using the make validate target.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM from a specification perspective. (That is, I didn't check the changes match the Elasticsearch PRs.) Not backporting indeed because:

  1. part of the change are 9.3 only and
  2. changing the optionality of location, model_id and project_id is breaking for language clients.

@Jan-Kazlouski-elastic
Copy link
Contributor Author

Thanks! LGTM from a specification perspective. (That is, I didn't check the changes match the Elasticsearch PRs.) Not backporting indeed because:

  1. part of the change are 9.3 only and
  2. changing the optionality of location, model_id and project_id is breaking for language clients.

Thank you @pquentin . One of the PRs to the elasticsearch is still under review, so this specification update will have to wait a bit till it's merged.

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, left a few suggestions

/**
* The name of the location to use for the inference task.
* The name of the Google Model Garden Provider for `completion` and `chat_completion` tasks.
* In order for Google Model Garden endpoint to be used `provider` must be defined and be other than `google`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this read: In order for a...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct. Fixed


export enum GoogleModelGardenProvider {
google,
anthropic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we merged anthropic for 9.2 we'll need to release docs for 9.2 that state that only anthropic is supported.

How about we modify this to only include anthropic and then create a new PR that adds the others? That way it'll make it easier to backport the 9.2 anthropic docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all providers except anthropic and google. Could you tell whether or not this PR will have to be backported @jonathan-buttner ?

/**
* For a `completion` or `chat_completion` task, allows setting up the `max_tokens` field for request to the Google Model Garden's Anthropic provider.
* If `max_tokens` is specified - it must be a positive integer.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an @ext_doc_id link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a link to anthropic documentation and clarified the comment.

{
"service": "googlevertexai",
"service_settings": {
"provider": "meta",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a remind to hold off on adding these until we merge the 9.2 version of the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the examples with meta provider. Will add them later.

@Jan-Kazlouski-elastic
Copy link
Contributor Author

Hello @jonathan-buttner
Your comments are addressed. Could you please check the tags for the PR? It says skip-backport, but you've mentioned backporting in one of your comments. I wouldn't want this to be missed.

@jonathan-buttner
Copy link
Contributor

@pquentin

changing the optionality of location, model_id and project_id is breaking for language clients.

Just to confirm since the initial ES PR that was merged in 9.2 changed optionality of those field does that mean we cannot backport this PR to 9.2?

@Jan-Kazlouski-elastic
Copy link
Contributor Author

@pquentin Could you please take a look at @jonathan-buttner 's question above?

@pquentin
Copy link
Member

(Answered offline, sorry for not doing it earlier.)

@pquentin pquentin added backport 9.2 and removed skip-backport This pull request should not be backported labels Oct 23, 2025
@Jan-Kazlouski-elastic Jan-Kazlouski-elastic merged commit 7f7d7fc into main Oct 23, 2025
8 checks passed
@Jan-Kazlouski-elastic Jan-Kazlouski-elastic deleted the google-model-garden-integration branch October 23, 2025 13:07
@github-actions
Copy link
Contributor

The backport to 9.2 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.2 9.2
# Navigate to the new working tree
cd .worktrees/backport-9.2
# Create a new branch
git switch --create backport-5423-to-9.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7f7d7fc312a3f4b4c5baeea7c3764b5716a50dca
# Push it to GitHub
git push --set-upstream origin backport-5423-to-9.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.2

Then, create a pull request where the base branch is 9.2 and the compare/head branch is backport-5423-to-9.2.

Jan-Kazlouski-elastic added a commit that referenced this pull request Oct 23, 2025
…sks (#5423)

* Add Google Model Garden support for completion and chat_completion tasks

* Fix comments

---------

Co-authored-by: Quentin Pradet <[email protected]>

(cherry picked from commit 7f7d7fc)
Jan-Kazlouski-elastic added a commit that referenced this pull request Oct 23, 2025
…sks (#5423) (#5530)

* Add Google Model Garden support for completion and chat_completion tasks

* Fix comments

---------

Co-authored-by: Quentin Pradet <[email protected]>

(cherry picked from commit 7f7d7fc)
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.

3 participants