fix(llms): inline text files for text-only models#5834
Open
pragnyanramtha wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBaseLLM adds support for inlining text-file attachments for non-multimodal models. A MIME-type allowlist and _is_text_file identify text files; _process_text_files_for_text_only_model reads files via read_text(), appends "Attached file: {name}" sections to message content, and removes the files field. Tests cover accepted text and rejected images. ChangesText file support for non-multimodal models
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/tests/llms/test_multimodal.py (1)
176-199: ⚡ Quick winAdd the complementary non-text rejection regression in this same block.
This new test covers success for
TextFile, but the contract also depends on non-text inputs still failing on text-only models. Adding an adjacentpytest.raises(ValueError)case (e.g.,ImageFile) will lock in both sides of the behavior.🧪 Suggested test addition
class TestOpenAIMultimodal: @@ def test_non_multimodal_model_accepts_text_files(self) -> None: """Test text files are inlined for models without multimodal support.""" @@ assert result == [ { "role": "user", "content": ( "Summarize the attached file.\n\n" "Attached file: readme\n" "hello from a text file" ), } ] + + def test_non_multimodal_model_rejects_non_text_files(self) -> None: + """Test non-text files still fail for models without multimodal support.""" + llm = LLM(model="openai/gpt-3.5-turbo") + messages = [ + { + "role": "user", + "content": "Describe this image.", + "files": {"image": ImageFile(source=MINIMAL_PNG)}, + } + ] + + with pytest.raises(ValueError, match="does not support multimodal input"): + llm._format_messages(messages)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/llms/test_multimodal.py` around lines 176 - 199, Extend the existing test_non_multimodal_model_accepts_text_files by adding a complementary negative case that asserts non-text files are rejected for text-only models: call llm._format_messages (same LLM(model="openai/gpt-3.5-turbo")) with a message that contains an ImageFile (or other non-text file type) in the files dict and wrap the call in pytest.raises(ValueError) to ensure a ValueError is thrown; reference the existing test_non_multimodal_model_accepts_text_files, the LLM._format_messages method, TextFile and ImageFile symbols, and pytest.raises(ValueError) so both acceptance and rejection behaviors are locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/llms/base_llm.py`:
- Around line 825-833: Normalize the MIME type string before matching by
stripping any parameters and lowercasing it: after fetching content_type =
getattr(file_input, "content_type", "") (and confirming it's a str), replace
content_type with content_type.split(";", 1)[0].strip().lower() and then perform
the existing checks (callable(getattr(file_input, "read_text", None)) and
(content_type.startswith("text/") or content_type in _TEXT_FILE_CONTENT_TYPES));
reference symbols: content_type, file_input, read_text, and
_TEXT_FILE_CONTENT_TYPES.
---
Nitpick comments:
In `@lib/crewai/tests/llms/test_multimodal.py`:
- Around line 176-199: Extend the existing
test_non_multimodal_model_accepts_text_files by adding a complementary negative
case that asserts non-text files are rejected for text-only models: call
llm._format_messages (same LLM(model="openai/gpt-3.5-turbo")) with a message
that contains an ImageFile (or other non-text file type) in the files dict and
wrap the call in pytest.raises(ValueError) to ensure a ValueError is thrown;
reference the existing test_non_multimodal_model_accepts_text_files, the
LLM._format_messages method, TextFile and ImageFile symbols, and
pytest.raises(ValueError) so both acceptance and rejection behaviors are locked
in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14f89687-655b-4400-bc8f-16ebc2278b26
📒 Files selected for processing (2)
lib/crewai/src/crewai/llms/base_llm.pylib/crewai/tests/llms/test_multimodal.py
pragnyanramtha
force-pushed
the
pragnyan/inline-text-files-text-only-models-5137
branch
from
fe782e9 to
4699ad2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
input_filesinto the prompt when the selected model is not multimodalopenai/gpt-3.5-turboFixes #5137.
Context
input_filescurrently trips the non-multimodal guard before file content is considered. Text files do not require vision support, so users hit the multimodal error even though the file can be represented as ordinary prompt text.This patch keeps image/PDF/non-text attachments on the existing vision-model path, while converting text-like files into appended prompt text for text-only models.
Validation
uv run pytest lib/crewai/tests/llms/test_multimodal.py::TestOpenAIMultimodal::test_non_multimodal_model_accepts_text_files lib/crewai/tests/llms/test_multimodal.py::TestOpenAIMultimodal::test_does_not_support_gpt35 -q-> 2 passeduv run ruff check lib/crewai/src/crewai/llms/base_llm.py lib/crewai/tests/llms/test_multimodal.py-> passeduv run ruff format --check lib/crewai/src/crewai/llms/base_llm.py lib/crewai/tests/llms/test_multimodal.py-> passeduv run mypy lib/crewai/src/crewai/llms/base_llm.py-> passedgit diff --check-> passedI also attempted the full
lib/crewai/tests/llms/test_multimodal.pyfile in this checkout, but optional LiteLLM / Google GenAI provider extras are not installed here; the resulting failures were missing-provider setup failures rather than this text-file path.Summary by CodeRabbit
New Features
Tests