-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support text, JSON, XML and YAML DocumentUrl
and BinaryContent
on OpenAI
#2851
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
Support text, JSON, XML and YAML DocumentUrl
and BinaryContent
on OpenAI
#2851
Conversation
@pulphix Thanks Fabio, I'll hold off on reviewing this pending #1161 (comment). |
0125d8a
to
2ff0bfd
Compare
9883cd8
to
affe38a
Compare
@DouweM @Kludex I updated the PR by removing the Magic Classes and porting the logic to I’m wondering whether it makes sense to include the file name in the
If multiple text files are uploaded as binary, they will all share the same name, which might create issues with LLM reasoning. |
ProposalAdd an optional
|
414cc86
to
83b8e15
Compare
Removed reference to filename and added URL for DocumentUrl. |
tests/models/cassettes/test_model_names/test_known_model_names.yaml
Outdated
Show resolved
Hide resolved
476cd9d
to
f7ef1ae
Compare
0e3214d
to
9885418
Compare
@DouweM I rebased from the main branch, but I encountered some failures. |
9885418
to
5b60fc0
Compare
5b60fc0
to
c0c5775
Compare
@staticmethod | ||
def _inline_file_block(media_type: str, text: str, identifier: str | None) -> str: | ||
id_attr = f' id="{identifier}"' if identifier else '' | ||
return ''.join(['-----BEGIN FILE', id_attr, ' type="', media_type, '"-----\n', text, '\n-----END FILE-----']) |
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.
return ''.join(['-----BEGIN FILE', id_attr, ' type="', media_type, '"-----\n', text, '\n-----END FILE-----']) | |
return '\n'.join([ | |
f'-----BEGIN FILE{id_attr} type="{media_type}"-----', | |
text, | |
f'-----END FILE {id_attr}-----', | |
]) |
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.
Done
|
||
@staticmethod | ||
def _inline_file_block(media_type: str, text: str, identifier: str | None) -> str: | ||
id_attr = f' id="{identifier}"' if identifier else '' |
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.
Would there always be an identifier?
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.
@DouweM based on the DocumentUrl object identifier can be None
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.
Hmm, I agree the type states it can be, but the implementation ensures it always has a value, as far as I can see. Can we add an assert identifier is not None
in here, as there's not much use in giving the model an inline text part if there is no way to identify it?
tests/models/test_openai.py
Outdated
|
||
model = OpenAIChatModel('gpt-4o', provider=OpenAIProvider(api_key=openai_api_key)) | ||
# Monkeypatch the client's create method | ||
model.client.chat.completions.create = fake_create |
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.
In this particular case I think it's acceptable to directly call the private _map_user_message
method (and add a # type: ignore[reportPrivateUsage]
comment) as it's a lot easier to follow than this mocking.
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.
Done
tests/models/test_openai.py
Outdated
assert parts[0]['type'] == 'text' | ||
text = parts[0]['text'] | ||
assert text.startswith(f'-----BEGIN FILE id="{identifier}" type="{media_type}"-----') | ||
assert text.rstrip().endswith('-----END FILE-----') |
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.
Can we do a direct assert text ==
so that we can verify the newlines etc?
assert_never(item) | ||
return chat.ChatCompletionUserMessageParam(role='user', content=content) | ||
# Fallback: unknown type — return empty parts to avoid type-checker Never error | ||
return [] |
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'd rather replace object
with a more specific type hint, so we can use assert_never(item)
here
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.
Done
tests/models/test_openai.py
Outdated
) | ||
|
||
|
||
async def test_openai_map_single_item_unknown_returns_empty_branch( |
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'm not sure why we need this test and the next one. What lines would be uncovered without them?
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.
Changed name of the test
@DouweM Updated PR based on requests. |
|
||
@staticmethod | ||
def _inline_file_block(media_type: str, text: str, identifier: str | None) -> str: | ||
id_attr = f' id="{identifier}"' if identifier else '' |
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.
Hmm, I agree the type states it can be, but the implementation ensures it always has a value, as far as I can see. Can we add an assert identifier is not None
in here, as there's not much use in giving the model an inline text part if there is no way to identify it?
…thub.com:pulphix/pydantic-ai into pulphix/implemented_text_file_support_for_openai
…implemented_text_file_support_for_openai
…implemented_text_file_support_for_openai
DocumentUrl
and BinaryContent
on OpenAI
@pulphix Thanks for your work on this Fabio! I made a couple more code organization tweaks and will get this out in a release today. |
Problem addressed
OpenAI rejects
text/plain
as file parts; Anthropic/Gemini accept document URLs directly.This created inconsistent DX.
New types
DocumentUrl
, adds optional filename and a magic marker.BinaryContent
, adds optional filename and a magic marker.Both preserve their original types in serialized history so users can filter for them.
OpenAI-specific handling
media_type == text/plain
:UserContent
with a clear delimiter:Other providers
Magic*
are effectively pass-through (treated like their base classes), so PDFs/text URLs keep working without special casing.Serialization/history
Magic*
classes in the message history (with anis_magic
marker) so users can filter by type even if OpenAI saw inline text at request time.Tests
Added tests ensuring:
MagicBinaryContent (text/plain)
→ inline text with delimiter on OpenAI.MagicBinaryContent (PDF)
→ file part on OpenAI.MagicDocumentUrl (text/plain)
→ inline text with delimiter on OpenAI (mocked download).MagicDocumentUrl (PDF)
→ file part on OpenAI (mocked download).All functional tests pass; repo’s global 100% coverage target remains managed outside our changes.
Example
examples/pydantic_ai_examples/magic_files.py
demonstrates bothMagicDocumentUrl
andMagicBinaryContent
with OpenAI and Anthropic.Loads API keys via python-dotenv if available (
load_dotenv()
).How to use
Import: