Skip to content

Conversation

Nymuxyzo
Copy link
Contributor

@Nymuxyzo Nymuxyzo commented Aug 17, 2025

Pull Request

Related issue

Fixes #626

What does this PR do?

  • Add tests

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Summary by CodeRabbit

  • New Features

    • View, update, and reset Typo Tolerance and Faceting settings on indexes.
  • Tests

    • Expanded coverage for composite embedders with pooling and nested configurations.
    • Experimental feature helpers enabled globally in the test suite and a helper to toggle composite embedders added.
  • Chores

    • Project Ruby version set to 3.1.7.

Copy link

coderabbitai bot commented Aug 17, 2025

Walkthrough

Adds a .ruby-version file, inserts two comment section headers in lib/meilisearch/index.rb, adds a new spec covering composite embedders with pooling, includes ExperimentalFeatureHelpers globally in specs, and adds an enable_composite_embedders test helper.

Changes

Cohort / File(s) Summary
Environment file
./.ruby-version
New file specifying Ruby version 3.1.7.
Index file (comments only)
lib/meilisearch/index.rb
Inserts two comment headers: ### SETTINGS - TYPO TOLERANCE and ### SETTINGS - FACETING (documentation-only; no functional changes).
Specs: embedders (experimental)
spec/meilisearch/index/settings_spec.rb
Adds test #update_embedders with pooling and composite validating composite embedders with pooling and nested HuggingFace embedder configs.
Spec config
spec/spec_helper.rb
Includes ExperimentalFeatureHelpers into global RSpec config (config.include ExperimentalFeatureHelpers).
Spec helpers
spec/support/experimental_feature_helpers.rb
Adds def enable_composite_embedders(toggle) delegating to configure_feature('compositeEmbedders', toggle).

Sequence Diagram(s)

(omitted — changes are documentation/comments and test/helper additions; no new runtime control flow to diagram)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (1 passed, 3 warnings, 1 inconclusive)

❌ Failed checks (3 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning Although the PR adds tests and helper methods for experimental composite embedder features, it does not implement the actual production code changes required by linked issue #626 to introduce a new pooling parameter or composite source in the embedders API. The only modifications to lib/meilisearch/index.rb are comment headers, and no functional logic for pooling or composite embedders has been introduced. Therefore, the PR does not satisfy the core objectives of updating the embedders API to support Meilisearch v1.14.0 features. Implement the necessary code changes in the embedders API to support the pooling option for HuggingFace embedders and the composite source for search and indexing configurations, then ensure the added tests pass against this new functionality.
Out of Scope Changes Check ⚠️ Warning This PR introduces changes such as adding a .ruby-version file and section comment headings in index.rb that are unrelated to the linked issue’s objectives of supporting composite embedders and pooling. The .ruby-version configuration is an environment setup change that should be managed separately and not mixed with feature tests. These unrelated modifications add noise to the PR and could be better isolated in a separate commit or pull request. Please remove or separate the .ruby-version file and other documentation-only changes into their own PR to keep this branch focused exclusively on implementing and testing composite embedders and pooling support.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive While the description correctly references the related issue and broadly states that tests are added, it offers no detail about which tests, what behavior is being validated, or other modifications included in the PR. The current description simply states “Add tests,” which is a generic summary that does not convey the scope or intent of the changes. Although it is related to the changeset, its vagueness may hinder understanding by reviewers. Please expand the description to include specific details on the files and tests added, the behaviors being validated (composite embedders and pooling), and any relevant configuration or helper changes introduced in this PR.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly describes the main change as adding tests for composite embedders and pooling. It aligns directly with the PR’s objectives to validate composite embedder behavior. It is concise and free of unrelated noise or file lists.

Poem

I nibble at code in the eve-light,
Ruby set to three-one-seven tonight,
Tests for composites take a hop and a spin,
Helpers enabled — let experiments begin! 🐇✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1682a3 and f0d12fd.

📒 Files selected for processing (5)
  • .ruby-version (1 hunks)
  • lib/meilisearch/index.rb (2 hunks)
  • spec/meilisearch/index/settings_spec.rb (1 hunks)
  • spec/spec_helper.rb (1 hunks)
  • spec/support/experimental_feature_helpers.rb (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • lib/meilisearch/index.rb
🚧 Files skipped from review as they are similar to previous changes (4)
  • .ruby-version
  • spec/spec_helper.rb
  • spec/support/experimental_feature_helpers.rb
  • spec/meilisearch/index/settings_spec.rb
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

codecov bot commented Aug 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (0a4833c) to head (f0d12fd).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #644   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          806       806           
=========================================
  Hits           806       806           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Nymuxyzo Nymuxyzo force-pushed the feat/composite_embedders-pooling branch from 88c8da1 to d1682a3 Compare August 17, 2025 20:35
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
spec/support/experimental_feature_helpers.rb (1)

11-13: Optionally assert feature toggle succeeded to avoid flaky specs

Currently, failures from /experimental-features (e.g., wrong host/port, 4xx) are ignored. For more robust tests, consider raising on non-2xx responses inside configure_feature.

You can adapt configure_feature like this (outside the changed lines):

def configure_feature(attribute_to_toggle, toggle)
  uri = URI("http://#{ENV.fetch('MEILISEARCH_URL', 'localhost')}")
  uri.path = '/experimental-features'
  uri.port = ENV.fetch('MEILISEARCH_PORT', '7700')

  req = Net::HTTP::Patch.new(uri)
  req.body = { attribute_to_toggle => toggle }.to_json
  req.content_type = 'application/json'
  req['Authorization'] = "Bearer #{MASTER_KEY}"

  res = Net::HTTP.start(uri.hostname, uri.port) { |http| http.request(req) }
  unless res.is_a?(Net::HTTPSuccess)
    raise "Failed to toggle experimental feature #{attribute_to_toggle}=#{toggle}: #{res.code} #{res.body}"
  end
  res
end
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1091577 and 88c8da1.

📒 Files selected for processing (5)
  • .ruby-version (1 hunks)
  • lib/meilisearch/index.rb (2 hunks)
  • spec/meilisearch/index/settings_spec.rb (1 hunks)
  • spec/spec_helper.rb (1 hunks)
  • spec/support/experimental_feature_helpers.rb (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
spec/meilisearch/index/settings_spec.rb (4)
spec/support/experimental_feature_helpers.rb (1)
  • enable_composite_embedders (11-13)
lib/meilisearch/client.rb (1)
  • index (132-134)
lib/meilisearch/index.rb (2)
  • update_embedders (763-769)
  • embedders (759-761)
lib/meilisearch/models/task.rb (1)
  • await (114-121)
spec/spec_helper.rb (1)
lib/meilisearch/client.rb (1)
  • include (14-488)
🪛 GitHub Actions: Tests
spec/meilisearch/index/settings_spec.rb

[error] 876-876: RuboCop: Layout/TrailingWhitespace detected trailing whitespace.


[error] 878-878: RuboCop: Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash.


[error] 879-879: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 881-881: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 882-882: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 883-883: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 883-883: RuboCop: Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.


[error] 886-886: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 887-887: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 888-888: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 889-889: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 893-893: RuboCop: Layout/FirstHashElementIndentation: Indent the right brace the same as the first position after the preceding left parenthesis.


[error] 895-895: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 895-895: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 896-896: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


[error] 896-896: RuboCop: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

🔇 Additional comments (5)
.ruby-version (1)

1-1: Ruby version pin looks good

Pinning to Ruby 3.1.7 is fine for CI consistency.

spec/support/experimental_feature_helpers.rb (1)

11-13: Helper naming and usage are consistent with existing pattern

enable_composite_embedders mirrors enable_edit_documents_by_function and targets the expected compositeEmbedders flag. Good addition.

spec/spec_helper.rb (1)

81-81: Good: expose ExperimentalFeatureHelpers globally

Including ExperimentalFeatureHelpers here avoids per-spec boilerplate and aligns with other helper inclusions.

lib/meilisearch/index.rb (2)

577-595: Typo tolerance settings: API integration looks correct

  • Getter, updater (PATCH with transformed attributes), and reset map cleanly to the API.
  • Aliases (get_typo_tolerance, typo_tolerance=) match existing conventions.

Looks good.


596-614: Faceting settings: consistent with settings API patterns

  • Getter, updater (PATCH with transformed attributes), and reset are implemented consistently.
  • Aliases (get_faceting, faceting=) align with existing naming.

All good.

Copy link

@coderabbitai coderabbitai bot left a 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 (1)
spec/support/experimental_feature_helpers.rb (1)

11-13: Helper to toggle composite embedders is consistent and correct; consider a safe “with_…” wrapper

This mirrors the existing pattern and delegates to the shared configurator correctly. As an optional enhancement to avoid leaking global server state across examples, add a block-based helper that ensures the feature is reverted even if the spec fails.

Example addition (outside the selected lines):

def with_composite_embedders_enabled
  enable_composite_embedders(true)
  begin
    yield
  ensure
    enable_composite_embedders(false)
  end
end

Usage in specs:

with_composite_embedders_enabled do
  # your test logic that relies on composite embedders being enabled
end
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 88c8da1 and d1682a3.

📒 Files selected for processing (5)
  • .ruby-version (1 hunks)
  • lib/meilisearch/index.rb (2 hunks)
  • spec/meilisearch/index/settings_spec.rb (1 hunks)
  • spec/spec_helper.rb (1 hunks)
  • spec/support/experimental_feature_helpers.rb (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .ruby-version
🚧 Files skipped from review as they are similar to previous changes (2)
  • spec/meilisearch/index/settings_spec.rb
  • lib/meilisearch/index.rb
🔇 Additional comments (1)
spec/spec_helper.rb (1)

81-81: Global include of ExperimentalFeatureHelpers: LGTM

Makes the experimental toggle helpers available across specs and aligns with how other helper modules are included.

@Nymuxyzo Nymuxyzo force-pushed the feat/composite_embedders-pooling branch from d1682a3 to 5f0e635 Compare August 24, 2025 18:39
@curquiza curquiza requested a review from brunoocasali August 26, 2025 11:26
@Nymuxyzo Nymuxyzo force-pushed the feat/composite_embedders-pooling branch from 5f0e635 to f0d12fd Compare September 9, 2025 09:54
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing this tests!

bors merge

Copy link
Contributor

meili-bors bot commented Sep 9, 2025

@meili-bors meili-bors bot merged commit 2756898 into meilisearch:main Sep 9, 2025
9 checks passed
@Nymuxyzo Nymuxyzo deleted the feat/composite_embedders-pooling branch September 9, 2025 16:19
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.

[v1.14] Add composite embedders and pooling for huggingface embedders (experimental)
2 participants