PR-500 simplified client wrapper functions#562
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the client wrapper functions by introducing a unified helper to format OpenAI chat messages with various media types (image, audio, video) and by adding base64 conversion methods to the corresponding data types.
- Updated PIL image conversions by importing PILImage and aligning type hints.
- Added build_openai_chat_format and internal helper functions to process images, audio, and video.
- Extended data types with new to_base64_str methods for Image, Audio, and Video objects.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| clarifai/runners/utils/data_utils.py | Updated image conversion functions and added media processing functions for chat messages. |
| clarifai/runners/utils/data_types.py | Added to_base64_str utility methods for converting media bytes to base64 strings. |
Comments suppressed due to low confidence (2)
clarifai/runners/utils/data_utils.py:132
- [nitpick] Consider using a distinct local variable for the base64 string conversion in _process_audio instead of reassigning the parameter 'audio', to improve code clarity.
if audio.bytes:
clarifai/runners/utils/data_utils.py:158
- [nitpick] Consider using a separate variable to hold the base64 string in _process_video rather than overwriting the parameter 'video', for improved readability.
if video.bytes:
luv-bansal
left a comment
There was a problem hiding this comment.
Looks good, but not sure , should we add build_openai_chat_format function to utils/openai_convertor.py file?
| return True | ||
|
|
||
|
|
||
| def build_openai_chat_format(prompt: str, image: Image, images: List[Image], audio: Audio, |
There was a problem hiding this comment.
Not sure, do you think we could move this to here
| return True | ||
|
|
||
|
|
||
| def build_openai_chat_format(prompt: str, image: Image, images: List[Image], audio: Audio, |
There was a problem hiding this comment.
also should be rename this function it to build_openai_messages or just openai_messages like we have openai_response function here https://github.com/Clarifai/clarifai-python/blob/master/clarifai/runners/utils/openai_convertor.py#L98?
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes preprocessing of image, audio, and video inputs into SDK utilities and provides helpers to assemble OpenAI-compatible chat messages.
- Introduce
openai_convertor.pywithbuild_openai_messagesandis_openai_chat_format - Add
process_image,process_audio, andprocess_videoindata_utils.py - Extend data types with
to_base64_strmethods and update import inmodel_client.py
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| clarifai/runners/utils/openai_convertor.py | Added message builder and chat-format validator for multimodal inputs |
| clarifai/runners/utils/data_utils.py | Implemented processing helpers for Image, Audio, and Video |
| clarifai/runners/utils/data_types/data_types.py | Added to_base64_str to Image, Audio, and Video types |
| clarifai/client/model_client.py | Updated is_openai_chat_format import to the new converter |
Comments suppressed due to low confidence (2)
clarifai/runners/utils/openai_convertor.py:1
- [nitpick] The filename uses 'convertor' spelling; consider renaming to 'converter' for consistency with common terminology.
import time
clarifai/runners/utils/openai_convertor.py:173
- Add unit tests for multimodal message construction and validation (
process_image,process_audio,process_video,build_openai_messages,is_openai_chat_format) to ensure correct behavior across modalities.
def build_openai_messages(
| prompt: str, | ||
| image: Image, | ||
| images: List[Image], | ||
| audio: Audio, | ||
| audios: List[Audio], | ||
| video: Video, | ||
| videos: List[Video], | ||
| messages: List[Dict], |
There was a problem hiding this comment.
Consider making parameters optional with default values (e.g., prompt: str = '', images: List[Image] = None) to simplify calls when some modalities are unused.
| prompt: str, | |
| image: Image, | |
| images: List[Image], | |
| audio: Audio, | |
| audios: List[Audio], | |
| video: Video, | |
| videos: List[Video], | |
| messages: List[Dict], | |
| prompt: str = '', | |
| image: Image = None, | |
| images: List[Image] = [], | |
| audio: Audio = None, | |
| audios: List[Audio] = [], | |
| video: Video = None, | |
| videos: List[Video] = [], | |
| messages: List[Dict] = [], |
| import json | ||
| import math | ||
| import operator |
There was a problem hiding this comment.
[nitpick] Remove unused imports (json, math, operator) to reduce clutter, unless they are used elsewhere in this module.
| import json | |
| import math | |
| import operator |
| raise ValueError("Image has no bytes") | ||
| return PILImage.open(io.BytesIO(self.proto.base64)) | ||
|
|
||
| def to_base64_str(self) -> str: |
There was a problem hiding this comment.
[nitpick] The to_base64_str implementation is duplicated across Image, Audio, and Video; consider extracting a common mixin or utility to avoid code duplication.
| def to_base64_str(self) -> str: | ||
| if isinstance(self.proto.base64, bytes): | ||
| return self.proto.base64.decode('utf-8') | ||
| elif isinstance(self.proto.base64, str): | ||
| return self.proto.base64 | ||
| elif not self.proto.base64: | ||
| raise ValueError("Audio has no bytes") | ||
|
|
There was a problem hiding this comment.
similarly here for all to_base64_str methods
luv-bansal
left a comment
There was a problem hiding this comment.
I feel like to_base64_str implementation is currently wrong, because I'm sure self.proto.base64 is bytes and not a base64-encoded bytes. I know this confusion in naming is for long time in Clarifai
Minimum allowed line rate is |
Why
How
Tests