feat: composition over inheritance #236
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors the OpenAI SDK integration in posthog-python from inheritance to composition, making it more resilient to SDK updates and following better OOP practices.
- Implements wrapper classes in
posthog/ai/openai/openai_providers.pythat store and delegate to original OpenAI objects instead of inheriting - Adds defensive programming with
getattrchecks inposthog/ai/openai/openai.pyto verify attribute existence before wrapping - Adds support for newer OpenAI SDK features like
responses,reasoning_tokens, andcached_tokensacross all files - Improves error handling by implementing
__getattr__fallbacks for unhandled methods - Maintains backward compatibility by checking feature availability at runtime before attempting to use new SDK capabilities
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| self._original_chat = getattr(self, 'chat', None) | ||
| self._original_embeddings = getattr(self, 'embeddings', None) | ||
| self._original_beta = getattr(self, 'beta', None) | ||
| self._original_responses = getattr(self, 'responses', None) |
There was a problem hiding this comment.
style: Consider using a helper method to reduce code duplication between AzureOpenAI and AsyncAzureOpenAI for getting original objects
| if self._original_chat is not None: | ||
| self.chat = WrappedChat(self, self._original_chat) | ||
|
|
||
| if self._original_embeddings is not None: | ||
| self.embeddings = WrappedEmbeddings(self, self._original_embeddings) | ||
|
|
||
| if self._original_beta is not None: | ||
| self.beta = WrappedBeta(self, self._original_beta) | ||
|
|
||
| # Only add responses if available (newer OpenAI versions) | ||
| if self._original_responses is not None: | ||
| self.responses = WrappedResponses(self, self._original_responses) |
There was a problem hiding this comment.
style: Consider using a helper method to reduce code duplication between AzureOpenAI and AsyncAzureOpenAI for wrapping objects
rafaeelaudibert
left a comment
rafaeelaudibert
left a comment
There was a problem hiding this comment.
Not expert in this code, but makes sense. We'd benefit from tests showing this works as expected (the fallback behaviour)
posthog/ai/openai/openai.py
Outdated
| if self._original_beta is not None: | ||
| self.beta = WrappedBeta(self, self._original_beta) | ||
|
|
||
| # Only add responses if available (newer OpenAI versions) |
There was a problem hiding this comment.
Useless comment, the above comment explains it
There was a problem hiding this comment.
fair. Thats the ai adding random comments that i didnt reject like i usually do
| self._client = client | ||
| self._original = original_responses | ||
|
|
||
| def __getattr__(self, name): |
There was a problem hiding this comment.
Will this be called only in case there's no method, or is this called all the time? Do we have unit tests for this wrapper?
There was a problem hiding this comment.
we do have unit tests, just not integration tests that will run with different versions. i have tested that manually.
that method is only called if we do not have an override. it allows us to fall back to the original client if we do not support
| posthog_distinct_id: Optional ID to associate with the usage event. | ||
| posthog_trace_id: Optional trace UUID for linking events. | ||
| posthog_properties: Optional dictionary of extra properties to include in the event. | ||
| posthog_privacy_mode: Whether to store input and output in PostHog. |
There was a problem hiding this comment.
Are we removing args or just docs were wrong? If the former, we'd need a major release. I'll assume it's the latter
There was a problem hiding this comment.
this was incorrect docs. fixing
|
Agree with your comments @rafaeelaudibert I am the only one that works on this so far and have a local test file that i moved out of this repo (without pyttest). will move back in and join with pytest soon |
Support both new and old openai sdks (since resource change) by changing to a composition approach over inheritance. This means we do not need to import the resources and should be more defensive to dependency changes
fixes: #234