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

Add RouteLLM Prompt Driver #987

Closed
wants to merge 1 commit into from
Closed

Conversation

collindutter
Copy link
Member

@collindutter collindutter commented Jul 16, 2024

Describe your changes

Adds RouteLLMPromptDriver for using RouteLLM to route between a strong Prompt Driver and weak Prompt Driver.

Issue ticket number and link

NA


📚 Documentation preview 📚: https://griptape--987.org.readthedocs.build//987/

@collindutter
Copy link
Member Author

Should we consider other routing solutions before putting this in place?

Comment on lines +100 to +101
self.model = prompt_driver.model
self.tokenizer = prompt_driver.tokenizer
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels a little weird, but I'm not sure what the alternative is

Copy link
Contributor

@dylanholmes dylanholmes Jul 16, 2024

Choose a reason for hiding this comment

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

I agree that this kind of mutation inside of a _get_prompt_driver method is weird.

I think that model should be fixed to route-llm. Otherwise you'll get duplicate StartPromptEvent and FinishPromptEvent for the underlying model. (Like if you route to gpt-4o, then RouteLlmPromptDriver will emit StartPromptEvent(model="gpt-4o") and the underlying OpenAiChatPromptDriver will also do this.) Alternatively, you could disable the events for this prompt driver via a method override, but that feels wrong.

I think tokenizer should be left as None. Is there any reason why it can't be? I couldn't find a reason after searching for places where the field is referenced.

@collindutter collindutter force-pushed the feature/route-prompt-driver branch from 58cd7c1 to f609a5a Compare July 16, 2024 00:17
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
griptape/drivers/prompt/routellm_prompt_driver.py 77.77% 5 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@collindutter collindutter force-pushed the feature/route-prompt-driver branch from f609a5a to aefd735 Compare July 16, 2024 00:56
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

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

Really cool! 🍦

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Native function calling support to `OpenAiChatPromptDriver`, `AzureOpenAiChatPromptDriver`, `AnthropicPromptDriver`, `AmazonBedrockPromptDriver`, `GooglePromptDriver`, and `CoherePromptDriver`.
- `OllamaEmbeddingDriver` for generating embeddings with Ollama.
- `GriptapeCloudKnowledgeBaseVectorStoreDriver` to query Griptape Cloud Knowledge Bases.
- `RouteLLMPromptDriver` for using RouteLLM to route between a strong Prompt Driver and weak Prompt Driver.
Copy link
Contributor

Choose a reason for hiding this comment

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

In response to

Should we consider other routing solutions before putting this in place?

If you were referencing the choice of making a "composite/decorator" Prompt Driver: I really like the decorator pattern for this scenario. It seems ideal to allow users to use a familiar and existing api (Prompt Driver), especially since LLM routing would primarily be used as a mechanism for reducing costs that would be introduced after the initial business logic. Like first I'd implement my application and get it working, then I'd say wait I want to reduce my LLM costs. At that point all they need to do is swap out the Prompt Driver. Sounds like a good deal!

If you were referencing the choice of RouteLLM vs something else, you did put RouteLLM in the name, so it seems like nothing is preventing us from adding another Prompt Driver that does LLM routing differently.

)
router: str = field(kw_only=True, default="mf", metadata={"serializable": True})
threshold: float = field(kw_only=True, metadata={"serializable": True})
client: Controller = field(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should name this field controller instead of client. When I read client I thought this thing was actually calling a service, but its not, its all local.

Comment on lines +100 to +101
self.model = prompt_driver.model
self.tokenizer = prompt_driver.tokenizer
Copy link
Contributor

@dylanholmes dylanholmes Jul 16, 2024

Choose a reason for hiding this comment

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

I agree that this kind of mutation inside of a _get_prompt_driver method is weird.

I think that model should be fixed to route-llm. Otherwise you'll get duplicate StartPromptEvent and FinishPromptEvent for the underlying model. (Like if you route to gpt-4o, then RouteLlmPromptDriver will emit StartPromptEvent(model="gpt-4o") and the underlying OpenAiChatPromptDriver will also do this.) Alternatively, you could disable the events for this prompt driver via a method override, but that feels wrong.

I think tokenizer should be left as None. Is there any reason why it can't be? I couldn't find a reason after searching for places where the field is referenced.

@collindutter
Copy link
Member Author

Closing for now based on offline discussion.

@collindutter collindutter deleted the feature/route-prompt-driver branch August 21, 2024 17:31
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.

2 participants