-
Notifications
You must be signed in to change notification settings - Fork 162
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
WIP litellm integration #320
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it generally looks good:
- I wonder if there is a way to load the secrets directly from env vars instead of having to provide them in the function call.
- I'd also like to simplify passing in extra arguments but idk if sql functions allow
*args
or**kwargs
I guess not
( %s | ||
, 'hello world' | ||
, api_key=>%s | ||
, extra_options => jsonb_build_object('input_type', %s::text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is no way of directly taking any additional arguments as an "extra option"? without wrapping it in a jsonb and passing it as an extra param?
, extra_options => jsonb_build_object( | ||
'aws_access_key_id', %s::text, | ||
'aws_secret_access_key', %s::text, | ||
'aws_region_name', %s::text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to autoload these from env vars if they exist? same for the api_key
select vector_dims | ||
( | ||
ai.litellm_embed | ||
( %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda wonder why litellm has no provider argument, I feel like model names will collide eventually.
plpy.error(f"missing {secret_name} secret") | ||
# This line should never be reached, but it's here to make the type checker happy. | ||
return "" | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there existing code that depends on this condition throwing an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I still need to do a full audit of all of the callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now audited the callers - from what I can tell none of our code depends on this throwing an error.
The motivation for this change is that with litellm there is no sensible "default secret name" that we could configure because every provider has different conventions for secret naming. This motivated the change on line 29 to allow for secret_name_default
to be None
.
lookin' good |
ea8e0bf
to
366ae89
Compare
118c54b
to
bf5622b
Compare
TODOs: - tests for all integrations - fine-tune configuration parameters - fine-tune huggingface (cold-start?)
0dd9908
to
411f485
Compare
411f485
to
227b4ee
Compare
No description provided.