-
Notifications
You must be signed in to change notification settings - Fork 102
Add support for multi-modal search #712
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
Add support for multi-modal search #712
Conversation
WalkthroughAdds multimodal support: an experimental Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client App
participant SDK as SDK
participant MS as Meilisearch Server
rect `#E8F0FF`
note right of SDK: Update embedder settings (indexing/search fragments)
App->>SDK: update_embedder(settings with indexingFragments/searchFragments)
SDK->>MS: PUT /indexes/:uid/settings/embedders (JSON with fragments)
MS-->>SDK: 200 / Task info
SDK-->>App: Task response
end
rect `#F0FFF0`
note right of SDK: Toggle experimental multimodal feature
App->>SDK: ExperimentalFeatures.set_multimodal(true)
SDK->>MS: PATCH /experimental-features { multimodal: true }
MS-->>SDK: { multimodal: true, ... }
SDK-->>App: ExperimentalFeaturesResult
end
rect `#FFF8E8`
note right of SDK: Search including media payload
App->>SDK: SearchQuery.with_media(Value) + params
SDK->>MS: POST /indexes/:uid/search { media: {...}, ... }
MS-->>SDK: Search results
SDK-->>App: Results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/search.rs (3)
411-414: Media field shape looks good; keep as optional.Adds
media: Option<Value>with proper serde skipping. Consider documenting expected JSON shape (object of fragments) since this is experimental.
703-707: Builder ergonomics: consider a fallible generic helper.
with_media(Value)is fine. Optionally addtry_with_media<T: Serialize>(&self, media: &T) -> Result<&mut Self, serde_json::Error>orwith_media_json<T: Serialize>(&mut self, media: T)to avoid requiring callers to pre-buildValue.+impl<'a, Http: HttpClient> SearchQuery<'a, Http> { + /// Attach media fragments from any serializable structure. + pub fn with_media_json<'b, T: Serialize>( + &'b mut self, + media: T, + ) -> &'b mut SearchQuery<'a, Http> { + self.media = serde_json::to_value(media).ok(); + self + } +}
1110-1136: Serialization test is solid; add a negative case.Great direct check that
mediaserializes as expected. Consider an extra test assertingmediais omitted when unset..code-samples.meilisearch.yaml (1)
1944-1957: New sample aligns with API additions.Example shows
.with_media(json!(...))alongside hybrid search; this mirrors the new builder. Optionally preface with a note in docs that the multimodal experimental feature must be enabled.src/settings.rs (2)
150-157: Embedder fragments exposed—API surface looks correct.
indexing_fragmentsandsearch_fragmentsasOption<HashMap<String, EmbedderFragment>>with camelCase serialization aligns with expected wire format. Consider adding brief docs stating these are experimental.
3157-3241: Good serialization coverage for fragments.Test validates both
indexingFragmentsandsearchFragments, plusrequest/response. Consider also asserting a round‑tripfrom_value::<Embedder>(serialized)to catch deserialization regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.code-samples.meilisearch.yaml(1 hunks)src/features.rs(6 hunks)src/search.rs(4 hunks)src/settings.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T13:28:23.700Z
Learnt from: LukasKalbertodt
PR: meilisearch/meilisearch-rust#625
File: src/search.rs:368-370
Timestamp: 2025-06-12T13:28:23.700Z
Learning: In the Meilisearch Rust client, `SearchQuery` serializes its per-query federation settings under the key `federationOptions`; only the top-level multi-search parameter is named `federation`.
Applied to files:
src/search.rs
🧬 Code graph analysis (1)
src/search.rs (2)
src/indexes.rs (26)
client(186-188)client(232-234)client(320-322)client(376-378)client(428-430)client(473-475)client(522-525)client(556-558)client(634-636)client(700-702)client(970-972)client(1038-1040)client(1089-1091)client(1133-1135)client(1186-1188)client(1245-1247)new(81-89)new(1783-1790)new(1994-2000)index(2171-2171)index(2188-2189)index(2226-2226)index(2298-2298)index(2326-2326)index(2352-2352)index(2379-2379)src/features.rs (1)
new(56-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (7)
src/settings.rs (2)
2815-2815: Import addition OK.
use serde_json::json;required for tests.
159-163: Struct matches Meilisearch 1.16 specThe
EmbedderFragment { value: serde_json::Value }definition correctly wraps each fragment’s payload under a"value"key as required by the server schema. No changes needed.src/features.rs (5)
17-19: Backward-compatible result field.
multimodal: boolwith#[serde(default)]avoids breaking on older servers.
50-52: Request knob for multimodal.Optional
multimodalfield mirrors the response; consistent with existing flags.
64-65: Constructor default None—consistent.Keeps updates explicit.
149-152: Setter reads cleanly.
set_multimodalmatches existing builder style.
168-178: Test covers end-to-end toggle.Verifies update + get for
multimodal. Looks good.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
+ Coverage 85.90% 86.21% +0.30%
==========================================
Files 19 19
Lines 6079 6186 +107
==========================================
+ Hits 5222 5333 +111
+ Misses 857 853 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
curquiza
left a comment
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.
bors merge
|
Merge conflict. |
curquiza
left a comment
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.
bors merge
curquiza
left a comment
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.
bors merge
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/search.rs (3)
414-416: LGTM: Field declaration is correct.The
mediafield is properly typed asOption<Value>to handle flexible multimodal data, and the serde attribute ensures it's omitted whenNone. The implementation aligns with the multimodal search feature requirements.Consider expanding the documentation to include an example of the expected structure or link to Meilisearch documentation:
- /// Provides multimodal data for search queries. + /// Provides multimodal data for search queries. + /// + /// This experimental field accepts media fragments (images, video, etc.) for multimodal search. + /// The structure depends on the configured embedder. See the Meilisearch multimodal search documentation.
718-722: LGTM: Builder method implementation is correct.The method properly takes ownership of the
Valueparameter and integrates well with the builder pattern for method chaining.Consider enhancing the documentation with usage guidance:
- /// Attach media fragments to the search query. + /// Attach media fragments to the search query. + /// + /// # Example + /// + /// ``` + /// # use serde_json::json; + /// # use meilisearch_sdk::{client::Client, search::SearchQuery}; + /// # let client = Client::new("http://localhost:7700", Some("masterKey")).unwrap(); + /// # let index = client.index("movies"); + /// let media_data = json!({ + /// "image": "base64_encoded_image_data" + /// }); + /// let query = index.search().with_media(media_data).build(); + /// ```
1136-1162: LGTM: Serialization test is correct.The test properly validates that the
mediaparameter serializes correctly with the expected field name and preserves the nested JSON structure.Consider adding an integration test that executes a search with media against a real Meilisearch instance (if not already covered in other test files):
#[meilisearch_test] async fn test_search_with_media(client: Client, index: Index) -> Result<(), Error> { // Setup index with multimodal embedder configuration // Execute search with media parameter // Verify results }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.code-samples.meilisearch.yaml(1 hunks)src/search.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .code-samples.meilisearch.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T13:28:23.700Z
Learnt from: LukasKalbertodt
Repo: meilisearch/meilisearch-rust PR: 625
File: src/search.rs:368-370
Timestamp: 2025-06-12T13:28:23.700Z
Learning: In the Meilisearch Rust client, `SearchQuery` serializes its per-query federation settings under the key `federationOptions`; only the top-level multi-search parameter is named `federation`.
Applied to files:
src/search.rs
🧬 Code graph analysis (1)
src/search.rs (2)
src/indexes.rs (26)
client(186-188)client(232-234)client(320-322)client(376-378)client(428-430)client(473-475)client(522-525)client(556-558)client(634-636)client(700-702)client(970-972)client(1038-1040)client(1089-1091)client(1133-1135)client(1186-1188)client(1245-1247)new(81-89)new(1832-1839)new(2043-2049)index(2220-2220)index(2237-2238)index(2275-2275)index(2347-2347)index(2375-2375)index(2401-2401)index(2428-2428)src/features.rs (1)
new(56-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
src/search.rs (1)
470-470: LGTM: Constructor initialization is correct.The
mediafield is properly initialized toNonein the constructor, consistent with other optional fields in the struct.
|
Build succeeded: |
Pull Request
Related issue
Fixes #698
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.