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

VertexAI connection parameters #1934

Closed
wants to merge 1 commit into from
Closed

VertexAI connection parameters #1934

wants to merge 1 commit into from

Conversation

Irillit
Copy link
Contributor

@Irillit Irillit commented Feb 11, 2025

ProjectId:

  1. Passed as a parameter in the code
  2. Defined as an environment variable
  3. Defined as an environment variable as a part of Firebase Config
  4. Set by gcloud auth application-default login

Location:

  1. Passed as a parameter in the code
  2. Defined as an environment variable
  3. Default 'us-central1' region

Credentials:

  1. Created from a Service Account environment variable
  2. Set by gcloud auth application-default login

I don't see the cache (vertexClientFactoryCache in JS version) in initializer of VertexAI.
I doubt if we need credentials. If we have a GCLOUD_SERVICE_ACCOUNT_CREDS, it might be more logical to provide the service account itself to the VertexAI.

@github-actions github-actions bot added the python Python label Feb 11, 2025
models: list[str] = dataclasses.field(default_factory=list)


def get_project_from_firebase_config() -> str | None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we get location from Firebase Config? As I got, it might be there as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is vertex plugin, it should not be aware of Firebase config.


credentials = options.google_auth

sa_env = os.getenv(const.GCLOUD_SERVICE_ACCOUNT_CREDS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VertexAI accepts Service Account directly. Maybe we'd better provide it?
I also expect that GCLOUD_SERVICE_ACCOUNT_CREDS contains the path to the JSON key of the Service Account. Is that so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

GCLOUD_SERVICE_ACCOUNT_CREDS contains JSON blob of the service account credentials. It was added specifically to support environments where it's hard to write the credentials file.

process.env.GCLOUD_SERVICE_ACCOUNT_CREDS

# SPDX-License-Identifier: Apache-2.0

"""Constants for VertexAI plugin."""
GCLOUD_PROJECT = 'GCLOUD_PROJECT'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GOOGLE_CLOUD_PROJECT is also reserved by Firebase. Should we read from it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's skip GOOGLE_CLOUD_PROJECT for now. can revisit later.

@Irillit Irillit requested a review from pavelgj February 11, 2025 11:47
@Irillit
Copy link
Contributor Author

Irillit commented Feb 11, 2025

@pavelgj
I tried to repeat the order of parameter retrieval from the JS version of Genkit. Could you check if I get it right?

@yesudeep
Copy link
Contributor

Please use https://www.conventionalcommits.org/en/v1.0.0/ for your commit message format

@yesudeep yesudeep added this to the py-0.1.0 milestone Feb 11, 2025

class ModelReference(BaseModel):
name: str
info: ModelInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

model reference should also contain the config

config?: z.infer<CustomOptions>;

Copy link
Collaborator

@pavelgj pavelgj Feb 19, 2025

Choose a reason for hiding this comment

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

no need to add now, could you please just add a todo?

'gemini-1.0-pro': ModelReference(
name='vertexai/gemini-1.0-pro',
info=ModelInfo(
versions=['gemini-1.0-pro-001', 'gemini-1.0-pro-002'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's drop versions, we're thinking of doing that on the JS side as well



# Deprecated on 2/15/2025
SUPPORTED_V1_MODELS = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's drop 1.0 (already deprecated)

@Irillit Irillit mentioned this pull request Feb 19, 2025
4 tasks
@Irillit
Copy link
Contributor Author

Irillit commented Feb 20, 2025

In PR #2014 I did the same with the new plugin API.

@Irillit Irillit closed this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants