Add ChatAuto #38
Add ChatAuto #38
Conversation
|
|
||
|
|
||
| def ChatAuto( | ||
| provider: Optional[str] = None, |
There was a problem hiding this comment.
It'd be nice if this provided typing support for available options (perhaps via Literal)?
There was a problem hiding this comment.
Yes! Tried out an enum at one point as well. Didn't seem to make sense unless that enum becomes the official list used throughout.
List could get long(er than it already is), but something like:
SUPPORTED_PROVIDERS = Literal['openai', 'groq', ...]
def ChatAuto(
provider: Optional[SUPPORTED_PROVIDERS] = None,Maybe an official string enum to track all supported Chat models would make sense here. idk. Just want to avoid having too many lists of providers that will need to be updated.
There was a problem hiding this comment.
I don't mind a bit of repetition if it's straightforward to update -- if there's just a Literal alongside the _provider_chat_model_map with a comment about updating both that seems good enough?
| chat_fun = ChatAuto | ||
| assert_turns_system(chat_fun) | ||
| assert_turns_existing(chat_fun) | ||
|
|
There was a problem hiding this comment.
It would be great to have another test that fails of _provider_chat_model_map (or something like it) doesn't hold every Chat*() implementation. I just know I'll be adding Chat*() classes and will definitely forget to add ChatAuto() support.
There was a problem hiding this comment.
Great idea! Is there a place now that holds the official list of all Chat funcs? Would be great to centralize that if possible.
There was a problem hiding this comment.
No, I was thinking we'd just compare _provider_chat_model_map to dir(chatlas), grepping for anything that starts with ^Chat*
| "Provider name is required as parameter or `CHATLAS_CHAT_PROVIDER` must be set." | ||
| ) | ||
|
|
||
| kwargs |= dict( |
There was a problem hiding this comment.
Elsewhere in the codebase I use {**x, **y} to accumulate kwargs in situations like this, so I'd prefer it here as well.
There was a problem hiding this comment.
Can make sure this follows suit! Though I am also willing to go update those occurrences to utilize this operator :)
There was a problem hiding this comment.
Thanks for the link to the PEP. I don't feel super strongly either way, but I'm inclined to keep using {**x} since it feels more language-agnostic (i.e., less Python specific)
| if provider not in _provider_chat_model_map: | ||
| raise ValueError( | ||
| "Provider name is required as parameter or `CHATLAS_CHAT_PROVIDER` must be set." | ||
| ) |
There was a problem hiding this comment.
I think we need a different error when provider is None vs a (mis-specified) string
| ValueError | ||
| If no valid provider is specified either through parameters or environment variables. |
There was a problem hiding this comment.
| ValueError | |
| If no valid provider is specified either through parameters or environment variables. | |
| ValueError | |
| If no valid provider is specified either through parameters or environment variables. |
| Chat | ||
| A configured Chat instance for the specified provider. |
There was a problem hiding this comment.
| Chat | |
| A configured Chat instance for the specified provider. | |
| Chat | |
| A configured Chat instance for the specified provider. |
| **kwargs, | ||
| ) -> Chat: | ||
| """ | ||
| Factory function to create a Chat instance based on a provider specified in code or in an environment variable. |
There was a problem hiding this comment.
I don't factory function is the right term here
| Factory function to create a Chat instance based on a provider specified in code or in an environment variable. | |
| Create a Chat instance using a provider determined by environment variables. |
| """ | ||
| Factory function to create a Chat instance based on a provider specified in code or in an environment variable. | ||
|
|
||
| This function creates a Chat instance based on the specified provider, with optional system prompt and conversation turns. |
There was a problem hiding this comment.
| This function creates a Chat instance based on the specified provider, with optional system prompt and conversation turns. | |
| Create a Chat instance based on the specified provider, with optional system prompt and conversation turns. |
| The provider can be specified either through the function parameter or via the CHATLAS_CHAT_PROVIDER environment variable. | ||
| Additional configuration can be provided through kwargs or the CHATLAS_CHAT_ARGS environment variable (as JSON). This allows | ||
| you to easily switch between different chat providers by changing the environment variable without modifying your code. | ||
|
|
There was a problem hiding this comment.
I think here would be a good place to explain environment variables will win over kwargs (and the other args)
Issue #37
Simple abstraction to enable ability to swap providers and Chat args through environment variables.
Example use case:
Caveats: