-
Notifications
You must be signed in to change notification settings - Fork 3
Implements a unified encodeing/decodeing pipeline for llm
#442
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
Conversation
eb8680
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Two high-level questions in addition to the comments below:
- I'm not seeing the
type_to_encodable_typelogic for the case when the user typeTis apydantic.BaseModelsubtype, or any test cases. Is that case meant to be theobjectcase whereencodeanddecodeare identity mappings? Can you add tests that exercise this logic? - What happened to
deserialize? Have we decided to identify it withpydantic.BaseModel.model_validate_json?
Yes, will do.
Yes, from implementing this, a slight refinement on the design. Pydantic gives us: serialize: U -> json it could be that the serialize defop I've defined is misnamed, what it provides is actually serialize: U -> OpenAIMessage, for which we don't need an inverse. |
|
@eb8680 @jfeser after some iterations, whittled down The one case where we don't want identity is for images: @type_to_encodable_type.register(Image.Image)
class EncodableImage(_Encodable[Image.Image, ChatCompletionImageUrlObject]):
t = ChatCompletionImageUrlObject
@classmethod
def encode(cls, image: Image.Image) -> ChatCompletionImageUrlObject:
return {
"detail": "auto",
"url": _pil_image_to_base64_data_uri(image),
}
@classmethod
def decode(cls, image: ChatCompletionImageUrlObject) -> Image.Image:
image_url = image["url"]
if not image_url.startswith("data:image/"):
raise RuntimeError(
f"expected base64 encoded image as data uri, received {image_url}"
)
data = image_url.split(",")[1]
return Image.open(fp=io.BytesIO(base64.b64decode(data)))and on top of this, we want non-identity mappings for two more cases on top of this: We map these types to That being said, maybe this is incorrect? adding a test for this functionality (whether tools that return |
|
Test fails, as expected: |
eb8680
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datvo06
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM!
|
@jfeser any further comments before this lands? |
jfeser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, but otherwise looks good!
This PR refactors the encoding/decoding pipeline to go through a unified
encode/decodeoperation.effectful/handlers/llm/encoding.pyimplements an associated type morally equivalent to:it provides a function
type_to_encodable_typewhich does a sort of type-class resolution and returns an instance ofEncodablefor a given typeT.effectful/handlers/llm/provider.pynow uniformly constructs theseEncodable[T]objects and uses their encode and decode operations when calling the LLM.The high level pipeline to implement
Template.__call__(template, args):type_to_encodable_typeto obtain anEncodable[T]for each type in the signature of templateargsto pydantic typesserializeto map pydantic types toOpenAIMessagesdecodeto map llm provided pydantic types back to python user typesencodeto map result to a pydantic typeserializeto map result to OpenAIMessagetype_to_encodable_typeto getEncodablefor return typedecodeto map pydantic type to user typeI expect some discussion about the interface around
Tool- for this initial version, I kept most of the API and logic the same, but we might want to restructure it a bit.Another aspect is the encoding type resolution might be a bit overengineered. Currently it descends into types such as
tuple[Image.Image, Image.Image]to construct anEncodablethat does the right thing for nested types as well.