Client abstraction over API resources (prototype) #2
Conversation
| elif isinstance(auth, str): | ||
| self.auth = ApiKeyAuth(auth or openai.api_key) | ||
| else: | ||
| self.auth = auth |
There was a problem hiding this comment.
Wonder if we should just do validation here if it's not one of the supported auth types. Otherwise we're going to get an AttributeError when we do the self.auth.get_token() call on L125.
There was a problem hiding this comment.
We could. I think we are side-stepping the biggest issue (passing in a str for api key based auth), but I wouldn't mind adding more validation here.
I was planning to make a better diagnostics story (adding the __repr__ methods started it) that would make it easier to see how your client was configured - as well as how to deal with OAI or AOAI specific endpoints.
openai/client.py
Outdated
| raise ValueError( | ||
| f'Unknown `backend` {backend} - expected one of "azure" or "openai"' | ||
| ) | ||
| if not api_base and backend == 'azure': |
There was a problem hiding this comment.
What if I want to pass my azure api_base via environment variable?
There was a problem hiding this comment.
Yeah, that should work. Will fix.
There was a problem hiding this comment.
Ah, I now remember - the problem is that the default is openai, and for azure you always need to have overridden the value. I'll put a check to make sure the api_base isn't "https://api.openai.com/v1". I'll have to go back and re-untangle this mess I think.
openai/client.py
Outdated
| await openai.Completion.acreate(**kwargs), | ||
| ) | ||
|
|
||
| def chatcompletion(self, messages, **kwargs) -> openai.ChatCompletion: |
There was a problem hiding this comment.
nit: since we snake_case the image API names, this kind of feels like it should follow that as well
There was a problem hiding this comment.
Yeah, I do think we should review the names across the board. I used the convention of iter_ for server sent event streaming APIs and _<variation name> for "child"/custom actions beyond the base generation API. But that was just the first thing that popped into my head.
Add missing `azuread` api_type support (for environment variables) Co-authored-by: Krista Pratico <[email protected]>
Co-authored-by: Krista Pratico <[email protected]>
|
|
||
| self.api_base = api_base or openai.api_base | ||
| self.organization = organization or openai.organization | ||
| if self.backend == 'azure' and self.api_base == "https://api.openai.com/v1": |
There was a problem hiding this comment.
what do you think about only comparing the api_base string up until the v or excluding the version altogether? e.g. self.api_base.startswith("https://api.openai.com/v")? I'm not sure how often the version is bumped, but it could possibly save us a re-release...
Live (running against the actual service endpoints) tests added
* add edit, moderation, audio apis
add docstrings/type hints
Experiment showing what a client abstraction over the Stripe/ApiResource model could look like.