Skip to content

fix: preserve MiniMax video input through multimodal ingestion - #2452

Open
octo-patch wants to merge 1 commit into
MemTensor:mainfrom
octo-patch:octo/20261001-parameter-refresh-recvwLbxPyf1Oc
Open

octo-patch wants to merge 1 commit into
MemTensor:mainfrom
octo-patch:octo/20261001-parameter-refresh-recvwLbxPyf1Oc

Conversation

@octo-patch

Copy link
Copy Markdown
Contributor

Reason: MiniMax-M3 video input is rejected by the shared chat content schema, and the multimodal reader discards the video URL.

Add a typed video_url content part and preserve its URL and chat metadata through message expansion and memory-source serialization. Existing text and image routing remains covered by regression tests. This change preserves video provenance; it does not add video analysis or decoding.

Related: #1859 covers model defaults separately.

Validation:

  • Installed pre-commit hooks passed during git commit.
  • make format (passed; 632 files unchanged).
  • .venv/bin/pytest tests/mem_reader/test_video_input.py -q (6 passed; the original code failed 4 regression cases).
  • .venv/bin/pytest tests/mem_reader/ -q (77 passed).

Checklist:

  • Bug fix with no new dependencies.
  • Self-reviewed the source changes.
  • Added regression tests for validation and retained video provenance.
  • Documented the scope and actual validation above.

@Memtensor-AI Memtensor-AI added area:memory 记忆存储、检索、更新、召回逻辑 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Oct 1, 2026
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2452
Task: e820d5999a867f0e
Base: main
Head: octo/20261001-parameter-refresh-recvwLbxPyf1Oc

🔍 OpenCodeReview found 4 issue(s) in this PR.


1. src/memos/types/openai_chat_completion_types/chat_completion_content_part_param.py (L13)

ChatCompletionContentPartVideoParam is added to the union type alias but is missing from __all__. Every other constituent type imported into this module (ChatCompletionContentPartImageParam, ChatCompletionContentPartInputAudioParam, etc.) is re-exported through __all__, so omitting the new type creates an inconsistency: from .chat_completion_content_part_param import * will silently skip ChatCompletionContentPartVideoParam, breaking any downstream code that relies on star-imports from this module (e.g., the __init__.py wildcard chain).

Add "ChatCompletionContentPartVideoParam" to __all__.

💡 Suggested Change

Before:

__all__ = ["ChatCompletionContentPartParam", "File", "FileFile"]

After:

__all__ = ["ChatCompletionContentPartParam", "ChatCompletionContentPartVideoParam", "File", "FileFile"]

2. src/memos/mem_reader/read_multi_modal/user_parser.py (L128-L129)

The video URL is stored in the generic content field, which is inconsistent with the image_url branch that stores the URL in a dedicated image_path field. Downstream consumers that dispatch on source.type and read image_path for images will find no equivalent video_path for videos; they'll silently get None from source.image_path (or any video_path attribute) and may fall back to content unexpectedly.

For consistency and correctness, store the URL in a dedicated video_path extra field (matching the image_path convention) and leave content as None or omit it, as is done for the image branch.

💡 Suggested Change

Before:

                            content=video_info.get("url"),
                            video_info=video_info,

After:

                            video_path=video_info.get("url"),
                            video_info=video_info,

3. tests/mem_reader/test_video_input.py (L45-L46)

Direct index access video_messages[0] is not guarded by a prior length assertion. If _expand_multimodal_messages returns no list-content items (e.g., due to a regression), this raises an opaque IndexError instead of a descriptive test failure. Add assert len(video_messages) == 1 before this line so pytest reports a clear assertion error.

💡 Suggested Change

Before:

    assert len(video_messages) == 1
    assert video_messages[0] == {**message, "content": [video]}

After:

    assert len(video_messages) == 1, f"Expected 1 video message, got {len(video_messages)}: {video_messages}"
    assert video_messages[0] == {**message, "content": [video]}

4. tests/mem_reader/test_video_input.py (L53)

Direct chained index accesses memories[0].metadata.sources[0] have no prior length guards. If parse_fast returns an empty list, or if sources is empty, the test raises a bare IndexError rather than a descriptive assertion failure. Add explicit length assertions immediately before these accesses.

💡 Suggested Change

Before:

    source = SourceMessage.model_validate(memories[0].metadata.sources[0].model_dump())

After:

    assert len(memories) == 1, f"Expected 1 memory, got {len(memories)}"
    assert len(memories[0].metadata.sources) >= 1, "Expected at least one source"
    source = SourceMessage.model_validate(memories[0].metadata.sources[0].model_dump())

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (6/6 executed). memos_python_core/changed-repo-python: 6/6. Duration: 13s

Branch: octo/20261001-parameter-refresh-recvwLbxPyf1Oc

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:memory 记忆存储、检索、更新、召回逻辑 status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants