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

fix: Include vertex_ai_beta in vertex_ai param mapping/Do not use google auth project_id #4461

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

t968914
Copy link

@t968914 t968914 commented Jun 28, 2024

Title

Vertex_ai_beta chat completion does not work with provided project_id in model info because it is always overwritten by project_id returned from google.auth.default(). However, google.auth.default() returns None in cases where there is no service account. This will also not work when litellm is hosted in a seperate google project from vertex ai models. The project_id passed in as a param should take precedent to support this.

Relevant issues

Type

🐛 Bug Fix

Changes

  • Include vertex_ai_beta when checking for vertex_ai provider type to map params
  • Do not use google auth default credentials project_id

Copy link

vercel bot commented Jun 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2024 4:50pm

@t968914 t968914 changed the title Include vertex_ai_beta in vertex_ai param mapping fix: Include vertex_ai_beta in vertex_ai param mapping/Do not use google auth project_id Jun 28, 2024
@krrishdholakia
Copy link
Contributor

hey @t968914 should be fixed already with this - 6b14cf7

Waiting on a new release (v1.40.31). Should be live by EOD.

Feel free to reopen, if that doesn't fix it.

@krrishdholakia
Copy link
Contributor

Realized you had 2 changes here. Since we've addressed the default project issue, can you isolate the PR to

Include vertex_ai_beta when checking for vertex_ai provider type to map params

lgtm for merge

quota_project_id=project_id,
scopes=["https://www.googleapis.com/auth/cloud-platform"],
)
if project_id is None:
project_id = creds_project_id
Copy link
Author

Choose a reason for hiding this comment

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

Hi @krrishdholakia, I saw the change in

hey @t968914 should be fixed already with this - 6b14cf7

This part is in the else block and for the application default credential case when a service account isn't being used. The google auth default method will always return None for project_id in this case, and similarly write over the previous value passed into this method.

Copy link
Author

Choose a reason for hiding this comment

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

And thanks for taking a look at the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@krrishdholakia This is currently blocking us from being able to test vertex_ai_beta in local dev

Copy link
Author

Choose a reason for hiding this comment

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

Our models in production are also deployed in a different GCP project than our litellm proxy, so we are unable to use vertex_ai_beta in production either

Copy link
Contributor

Choose a reason for hiding this comment

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

noted - working on this today

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the review @andrewmjc @t968914

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.

None yet

3 participants