This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

Add HybridRecordEncoder #265

Merged

izellevy merged 9 commits into pinecone-io:main from izellevy:feature/hybrid_support

Jan 29, 2024

Collaborator

izellevy commented Jan 23, 2024 •

edited

Loading

Problem

We don't have support for Hybrid Search in canopy.

Solution

Adds a new HybridRecordEncoder that uses the given dense encoder and the BM25 sparse encoder for
hybrid search purposes.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

Added relevant tests.


          Add HybridRecordEncoder

42e69d6

izellevy requested review from acatav, miararoy and igiloh-pinecone

January 23, 2024 15:12

acatav approved these changes

View reviewed changes

Contributor

acatav left a comment

LGTM, see my small comments

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved

tests/unit/record_encoder/test_hybrid_record_encoder.py Outdated Show resolved Hide resolved

tests/unit/record_encoder/test_hybrid_record_encoder.py Show resolved Hide resolved

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved

tests/unit/record_encoder/test_hybrid_record_encoder.py Show resolved Hide resolved

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved


          Fix structure

be077e3

igiloh-pinecone reviewed

View reviewed changes

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved

igiloh-pinecone reviewed

View reviewed changes

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved

igiloh-pinecone reviewed

View reviewed changes

src/canopy/models/data_models.py Show resolved Hide resolved

izellevy added 4 commits

January 24, 2024 17:04


          Fix PR comments

c6184d6


          Fix config

3c2fb86

Fix

02ccd91


          Fix docs

bf5ef6e

igiloh-pinecone suggested changes

View reviewed changes

Contributor

igiloh-pinecone left a comment

One more change.

(And one suggestion, that can be deferred to a separate PR)

src/canopy/knowledge_base/knowledge_base.py Show resolved Hide resolved

src/canopy/knowledge_base/record_encoder/hybrid.py Outdated Show resolved Hide resolved


          Move dense encoder

62e74b1

igiloh-pinecone reviewed

View reviewed changes

Contributor

igiloh-pinecone left a comment

@izellevy see a few last comments about unit tests

src/canopy/knowledge_base/record_encoder/hybrid.py

+                          **kwargs: Additional arguments to pass to the RecordEncoder.
+                      """  # noqa: E501
+                      if not 0 < alpha <= 1:

Contributor

igiloh-pinecone Jan 28, 2024

We should probably add an explicit error message for alpha==0, saying "sparse only not supported"

Collaborator Author

izellevy Jan 28, 2024

Added

tests/unit/record_encoder/test_hybrid_record_encoder.py Outdated Show resolved Hide resolved

tests/unit/record_encoder/test_hybrid_record_encoder.py Outdated Show resolved Hide resolved


          Fix comments

190618a

igiloh-pinecone approved these changes

View reviewed changes

Contributor

igiloh-pinecone left a comment

LGTM

Fix

dfa0692

izellevy added this pull request to the merge queue

Merged via the queue into pinecone-io:main with commit 444804b

7 checks passed

izellevy deleted the feature/hybrid_support branch

January 29, 2024 10:24

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet