Skip to content

fix: preserve video content parts in chat history - #274

Closed
octo-patch wants to merge 1 commit into
Eigenwise:mainfrom
octo-patch:octo/20260811-input-capability-recvrq4PwinjrT
Closed

fix: preserve video content parts in chat history#274
octo-patch wants to merge 1 commit into
Eigenwise:mainfrom
octo-patch:octo/20260811-input-capability-recvrq4PwinjrT

Conversation

@octo-patch

Copy link
Copy Markdown
Contributor

Reason: MiniMax-M3 accepts video input, but ChatHistory currently serializes video content parts as JSON text.

  • Detect valid video_url content parts during recursive multimodal extraction.
  • Preserve native content-part dictionaries during context token serialization.
  • Add regression coverage for chat history and token counting.

Checks:

  • uv run --frozen black --check atomic-agents/atomic_agents/context/chat_history.py atomic-agents/atomic_agents/agents/atomic_agent.py atomic-agents/tests/context/test_chat_history.py atomic-agents/tests/agents/test_atomic_agent.py
  • uv run --frozen flake8 --config .flake8 atomic-agents/atomic_agents/context/chat_history.py atomic-agents/atomic_agents/agents/atomic_agent.py atomic-agents/tests/context/test_chat_history.py atomic-agents/tests/agents/test_atomic_agent.py
  • uv run --frozen pytest -q atomic-agents/tests/context/test_chat_history.py atomic-agents/tests/agents/test_atomic_agent.py
  • uv run --frozen pytest -q atomic-agents
  • git diff --check

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, the MiniMax use case is real and _extract_multimodal_info is the right place to hook in. Two things before this can go in though.

The token-count path breaks at runtime. LiteLLM's token_counter only understands text and image_url content parts; a video_url part makes it raise. Repro against this branch:

from litellm import token_counter

token_counter(model="gpt-4o", messages=[{
    "role": "user",
    "content": [{"type": "video_url", "video_url": {"url": "https://example.com/v.mp4"}}],
}])
# ValueError: Invalid content item type: dict. Expected str or dict with 'type' field. ...

With the new elif isinstance(item, dict) branch in _serialize_history_for_token_count, that dict now reaches litellm as-is, so get_context_token_count() raises TokenCountError. And since _trim_context() calls it on every run() when max_context_tokens is set, video plus context trimming crashes every run. The test added for this asserts the serialized shape but never invokes the counter, which is why it stays green. Video parts should instead hit a text placeholder for a rough estimate, same spirit as the existing Image fallback a few lines down.

Detection by dict shape is something we'd rather avoid. With this change, any user schema field holding a dict that happens to match {"type": "video_url", "video_url": {"url": str}} silently gets hoisted out of the JSON payload into a content part. That's surprising at a distance and untyped, which cuts against the framework's schema-first design.

The direction we'd like instead: a typed VideoURL class mirroring instructor's Image API (from_url(), to_openai() emitting the video_url part), treated as a first-class multimodal type in _extract_multimodal_info, with get_history() appending video.to_openai() (instructor passes plain dicts through to the provider untouched, so this works on the wire). Users then declare video: VideoURL in their schema instead of a bare dict. Instructor itself has no Video type yet (checked 1.14.x/1.15.x and the docs), so I've opened an upstream discussion asking for one: 567-labs/instructor#2520. Once that lands we can swap it in behind the same seam.

Would you be up for reworking the PR in that direction? Happy to help with the details, or we can take it over the line ourselves with you credited.

@Eigenwise

Copy link
Copy Markdown
Owner

Went ahead with the typed approach in #276 and credited you as co-author on the commit. Once that merges this one can close. Thanks again for surfacing the use case.

@Eigenwise

Copy link
Copy Markdown
Owner

Superseded by #276, which is now merged. VideoURL ships in the next release. Thanks again for the report and the groundwork.

@Eigenwise Eigenwise closed this Aug 11, 2026
pull Bot pushed a commit to bhardwajRahul/atomic-agents that referenced this pull request Aug 11, 2026
Providers like MiniMax accept OpenAI-compatible video_url content parts,
but Instructor has no video type (567-labs/instructor#2520). VideoURL
fills the gap: ChatHistory extracts it like Instructor's Image/Audio/PDF
and get_history() emits its content part, which Instructor forwards to
the provider unchanged. Token counting turns video parts into text
placeholders because LiteLLM's token_counter raises on video_url parts.

Supersedes the raw-dict approach from Eigenwise#274.

Closes Eigenwise#275

Co-Authored-By: octo-patch <266937838+octo-patch@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants