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

refactor: add batch_size to FAISS __init__ #6401

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

pandasar13
Copy link
Contributor

Related Issues

Proposed Changes:

Added batch_size to the init method of FAISSDocumentStore and modified the functions that use batch_size to either specify batch_size or use self.batch_size.

This way batch_size can be specified in the init method, being consistent with other document stores (e.g. Weaviate, Opensearch)

How did you test it?
manual verification

Notes for the reviewer
Checklist
I have read the contributors guidelines and the
code of conduct
I have updated the related issue with new insights and changes
I added unit tests and updated the docstrings
I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
I documented my code
I ran pre-commit hooks and fixed any issue

@pandasar13 pandasar13 requested a review from a team as a code owner November 23, 2023 15:36
@pandasar13 pandasar13 requested review from julian-risch and removed request for a team November 23, 2023 15:36
@anakin87 anakin87 requested review from anakin87 and removed request for julian-risch November 23, 2023 15:39
@julian-risch
Copy link
Member

julian-risch commented Nov 23, 2023

@pandasar13 Could you please move the release note you created to releasenotes//notes/add-batch_size_faiss_init-5e97c1fb9409f873.yaml Currently it is in releasenotes/notes/releasenotes/notes/add-batch_size_faiss_init-5e97c1fb9409f873.yaml
In the note, you can leave out all the other sections and just put your note in the enhancements sections. We use the upgrade section for changes where we would like to advise the users how to migrate.

@coveralls
Copy link
Collaborator

coveralls commented Nov 23, 2023

Pull Request Test Coverage Report for Build 6972059698

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 143 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 40.214%

Files with Coverage Reduction New Missed Lines %
nodes/file_converter/init.py 1 88.89%
document_stores/faiss.py 142 18.35%
Totals Coverage Status
Change from base Build 6971350954: 0.06%
Covered Lines: 10688
Relevant Lines: 26578

💛 - Coveralls

@anakin87 anakin87 requested a review from a team as a code owner November 23, 2023 16:03
@anakin87 anakin87 requested review from dfokina and removed request for a team November 23, 2023 16:03
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thanks for your 1st contribution to Haystack! 💟

I will merge this PR when the tests pass...

@anakin87 anakin87 merged commit edb40b6 into deepset-ai:main Nov 23, 2023
65 checks passed
@pandasar13 pandasar13 deleted the faiss_refactor branch November 24, 2023 10:08
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.

Add batch_size to FAISS __init__
4 participants