-
Notifications
You must be signed in to change notification settings - Fork 18.3k
refactor(core): standard content blocks #32085
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Like block_type
, don't think we should use it on annotations. Have some remaining questions about multimodal
|
||
|
||
def is_data_content_block( |
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.
I don't mind updating the types but I don't think we should break untyped dicts in these formats in 0.4 if we can avoid it. If we decide to migrate IMO we should retain backward compatibility and emit deprecation warnings in the appropriate code paths (you may be able to just add a warning to is_data_content_block
but we'll need to check integrations).
… update ReasoningContentBlock structure
…reat each step as a distinct block in a list
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.
Nice progress, some nits
"""Signature of the reasoning. | ||
|
||
Inspired by: | ||
- https://ai.google.dev/gemini-api/docs/thinking#signatures |
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.
AFAIK Gemini doesn't include signature
on thinking parts, rather on text with thought=False
or function calls.
CodSpeed WallTime Performance ReportMerging #32085 will not alter performanceComparing
|
CodSpeed Instrumentation Performance ReportMerging #32085 will not alter performanceComparing Summary
|
mime_type: Literal["text/plain"] | ||
"""MIME type of the file. Required for base64.""" | ||
|
||
base64: str |
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.
Required base64
field conflicts with optional text
field. If text
is optional when base64 is provided, then base64
should probably be NotRequired
as well to allow for text-only usage.
base64: str | |
base64: NotRequired[str] |
Copilot uses AI. Check for mistakes.
base64: str | ||
"""Data as a base64 string.""" |
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.
Required base64
field is inconsistent with the presence of optional file_id
field. If content can be provided via file_id
, then base64
should be NotRequired
to allow for ID-only usage.
base64: str | |
"""Data as a base64 string.""" | |
base64: NotRequired[str] | |
"""Data as a base64 string. Optional if `file_id` is provided.""" |
Copilot uses AI. Check for mistakes.
except ValidationError: | ||
return False | ||
else: | ||
return True | ||
|
||
|
||
# These would need to be refactored | ||
def convert_to_openai_image_block(content_block: dict[str, Any]) -> dict: | ||
"""Convert image content block to format expected by OpenAI Chat Completions API.""" | ||
if content_block["source_type"] == "url": |
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 function references the old source_type
field which has been removed in the refactor. This will cause a KeyError when called. The function needs to be updated to work with the new unified type system.
Copilot uses AI. Check for mistakes.
|
||
Returns: | ||
True if the content block is a data content block, False otherwise. | ||
""" | ||
try: | ||
_ = _DataContentBlockAdapter.validate_python(content_block) | ||
_DataAdapter.validate_python(block) | ||
except ValidationError: | ||
return False |
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.
If we return False or block.get("type") in ("audio", "image", "video)" and "source_type" in block
, I believe we keep backwards compat.
block_type
replaces both oldtype
andsource_type
fieldsblock_type
, instead of bothtype
andsource_type
Literal
value inblock_type
directly maps to exactly oneTypedDict
.data:file:id
channel, you simplify adapters likeconvert_to_openai_data_block
, and avoid boilerplate branching on media kindmime_type
field or introduce a new discriminator (e.g.data:image:id
). But until there's a strong, real-world need to distinguish "ID for image" vs. "ID for audio," the singlefile:id
variant keeps the schema lean