Skip to content
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

fix: Cohere inconsistent embeddings and documents lengths #284

Merged
merged 9 commits into from
Feb 6, 2024
Merged

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented Jan 29, 2024

I am looking into this issue #283 and found a bug that I think is unrelated.

Update:
Running the code in issue #283 shows that the changes in this branch do seem to help. See comment here.

Basically we were copying the embeddings so we'd end up with twice as many embeddings as documents. I don't think this will fix the error brought up in issue #283, but we should avoid doing this.

Additional:

  • also fixed some failing integration tests

@sjrl sjrl requested a review from a team as a code owner January 29, 2024 13:11
@sjrl sjrl requested review from ZanSara and removed request for a team January 29, 2024 13:11
@sjrl
Copy link
Contributor Author

sjrl commented Jan 29, 2024

Hey @ZanSara I'm noticing that the integration tests that require a Cohere API key are not running in CI. Is this on purpose or should we be able to run them?

@sjrl sjrl changed the title fix: Don't double length of embeddings vs documents fix: Inconsistent embeddings and documents sizes Jan 29, 2024
@sjrl sjrl changed the title fix: Inconsistent embeddings and documents sizes fix: Inconsistent embeddings and documents lengths Jan 29, 2024
@ZanSara
Copy link
Contributor

ZanSara commented Feb 2, 2024

Hey @sjrl, thank you for this PR! Let's split the two things though:

  1. the bugfix in one PR, so I can approve it right away, and
  2. issue + PR for the Cohere tests, we need to address that separately and other tests might need fixing

Thank you for your help!

@sjrl sjrl mentioned this pull request Feb 5, 2024
@sjrl sjrl changed the title fix: Inconsistent embeddings and documents lengths fix: Cohere inconsistent embeddings and documents lengths Feb 5, 2024
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Testing out the example code the improvement is evident! Thank you for fixing this one! 🤗

@ZanSara ZanSara merged commit 2fc2676 into main Feb 6, 2024
9 checks passed
@ZanSara ZanSara deleted the cohere-fix branch February 6, 2024 10:15
@illorca-verbi
Copy link
Contributor

Thanks for this fix. When will the release be cut for https://pypi.org/project/cohere-haystack/?

@anakin87
Copy link
Member

anakin87 commented Feb 9, 2024

released: https://pypi.org/project/cohere-haystack/0.3.1/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants