Skip to content

Revert: feat(embedder): Gemini Embedding 2 multimodal support (#607)#703

Merged
qin-ctx merged 1 commit intomainfrom
revert-pr-607
Mar 17, 2026
Merged

Revert: feat(embedder): Gemini Embedding 2 multimodal support (#607)#703
qin-ctx merged 1 commit intomainfrom
revert-pr-607

Conversation

@qin-ctx
Copy link
Copy Markdown
Collaborator

@qin-ctx qin-ctx commented Mar 17, 2026

Summary

Reverts #607 due to multiple critical issues found during code review:

  1. embedding_config.py syntax errors - inserting gemini into provider/backend description strings broke string literals in multiple places, and a missing closing parenthesis on task_type field. This file cannot be parsed by Python at all, which breaks all embedding functionality for every provider (not just Gemini).

  2. EmbeddingMsg.init rejects new kwargs - the custom init overrides the dataclass-generated one but does not accept media_uri, media_mime_type, or id. Callers (from_dict(), from_context()) pass these params, causing TypeError at runtime.

  3. supports_multimodal never overridden - GeminiDenseEmbedder inherits supports_multimodal = False from base class. The multimodal branch in collection_schemas.py is gated by this flag, so the entire multimodal feature (the core purpose of this PR) is dead code.

  4. Security: ctx=None file read - viking_fs.read_file_bytes called with ctx=None bypasses tenant isolation in multi-tenant deployments.

See #607 comment for full review details.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 17, 2026 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@qin-ctx qin-ctx mentioned this pull request Mar 17, 2026
Merged
19 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 17, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

607 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • EmbeddingMsg still has leftover id handling in from_dict

Requires further human verification:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

AttributeError in EmbeddingMsg.from_dict

The from_dict method tries to access obj.id as a default value, but the EmbeddingMsg class no longer has an id attribute, causing an AttributeError when data does not contain an id key.

 
obj.id = data.get("id", obj.id)

@qin-ctx qin-ctx merged commit ae35f46 into main Mar 17, 2026
29 of 31 checks passed
@qin-ctx qin-ctx deleted the revert-pr-607 branch March 17, 2026 13:29
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 17, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 17, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore missing id field to EmbeddingMsg

The EmbeddingMsg class is missing the id field, which is used in from_dict and
serialization. This causes an AttributeError when data lacks an id key, and breaks
asdict serialization. Re-add the id field with default_factory to restore
functionality.

openviking/storage/queuefs/embedding_msg.py [4-14]

-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
 from typing import Any, Dict, List, Optional, Union
 from uuid import uuid4
 
 
+@dataclass
 class EmbeddingMsg:
     message: Union[str, List[Dict[str, Any]]]
     context_data: Dict[str, Any]
     telemetry_id: str = ""
     semantic_msg_id: Optional[str] = None
+    id: str = field(default_factory=lambda: str(uuid4()))
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the id field is missing from EmbeddingMsg, which would cause an AttributeError in from_dict when accessing obj.id as a default value. However, the proposed fix reverts to using @dataclass which may not align with the PR's intent to switch to a regular class.

Medium

ZaynJarvis pushed a commit to ZaynJarvis/OpenViking that referenced this pull request Mar 17, 2026
@qin-ctx @ZaynJarvis
Revert "feat(embedder): Gemini Embedding 2 multimodal support (text +…
eb5182e 
… image/video/audio/PDF) (volcengine#607)" (volcengine#703)

This reverts commit 95bd197.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhoujh01 zhoujh01 zhoujh01 approved these changes

Assignees

No one assigned

Labels

Review effort 3/5

Projects

OpenViking project
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@qin-ctx @CLAassistant @zhoujh01