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.

Sign up for GitHub

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

Jump to bottom

Vertex with openai #1084

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

Vertex with openai #1084

wants to merge 27 commits into from
+176 −43

Conversation

jayeshp19
Copy link
Collaborator

No description provided.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 30c4fcf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-plugins-openai Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

theomonnom
theomonnom approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@theomonnom theomonnom left a comment

Choose a reason for hiding this comment

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

can you add this LLM to our test_llm.py.
VertexAI is an important one :)

theomonnom
theomonnom reviewed Nov 19, 2024
View reviewed changes
livekit-plugins/livekit-plugins-openai/livekit/plugins/openai/_oai_api.py
Comment on lines +119 to +128
if arg_info.type is int and arg_info.choices and model is not None:
from .models import VertexModels

vertex_models_set = set(get_args(VertexModels))

if model in vertex_models_set:
raise ValueError(
f"Parameter '{arg_info.name}' uses 'choices' with 'int', which is not supported by Vertex AI."
)

Copy link
Member

@theomonnom theomonnom Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One issue I see with this approach is that we have to keep a up to date list of the supported models Google has. Maybe we can simply create a dataclass with supported stuff?

like:

@dataclass
class SupportedFunctionArguments:
 ....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good idea, I'll update pr

@theomonnom
Copy link
Member

Idk if this is the right approach: but our anthropic inject a user message if the chatcontext doesn't start with one

agents/livekit-plugins/livekit-plugins-anthropic/livekit/plugins/anthropic/llm.py

Line 300 in a5ca1a5

0, {"role": "user", "content": [{"type": "text", "text": "(empty)"}]}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbsp nbsp nbsp approved these changes

@theomonnom theomonnom theomonnom approved these changes

@davidzhao davidzhao Awaiting requested review from davidzhao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jayeshp19 @theomonnom @davidzhao @nbsp