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.

Sign up for GitHub

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

Jump to bottom

feat: SentenceTransformersTextEmbedder supports config_kwargs #8432

Merged
merged 6 commits into from
Oct 14, 2024
Merged

feat: SentenceTransformersTextEmbedder supports config_kwargs #8432

merged 6 commits into from
Oct 14, 2024

Conversation

alperkaya
Copy link
Contributor

Related Issues

Proposed Changes:

How did you test it?

CI, update unittests

Notes for the reviewer

Checklist

@alperkaya alperkaya requested a review from a team as a code owner October 1, 2024 15:43
@alperkaya alperkaya requested review from julian-risch and removed request for a team October 1, 2024 15:43
@coveralls
Copy link
Collaborator

coveralls commented Oct 1, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11330940911

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 90.332%
Files with Coverage Reduction New Missed Lines %
components/embedders/sentence_transformers_text_embedder.py 2 96.23%
Totals Coverage Status
Change from base Build 11330856805: 0.002%
Covered Lines: 7465
Relevant Lines: 8264
💛 - Coveralls

davidsbatista
davidsbatista reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@davidsbatista davidsbatista left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, just a small comment regarding the linter config you changed, and please also add the release-notes.

Thanks for your contribution!

pyproject.toml Outdated Show resolved Hide resolved
@davidsbatista davidsbatista mentioned this pull request Oct 2, 2024
Merged
@julian-risch julian-risch changed the title feat: add config_kwargs feat: SentenceTransformersTextEmbedder supports config_kwargs Oct 2, 2024
@alperkaya alperkaya requested a review from a team as a code owner October 2, 2024 14:41
@alperkaya alperkaya requested review from dfokina and removed request for a team October 2, 2024 14:41
julian-risch
julian-risch requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks quite good to me. My only suggestion is to extend the docstring. Will also be more consistent with the DocumentEmbedder then.

haystack/components/embedders/sentence_transformers_text_embedder.py Outdated Show resolved Hide resolved
julian-risch
julian-risch approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM

@davidsbatista davidsbatista enabled auto-merge (squash) October 14, 2024 15:53
@davidsbatista davidsbatista merged commit b40f0c8 into deepset-ai:main Oct 14, 2024
18 checks passed
LastRemote pushed a commit to LastRemote/haystack that referenced this pull request Oct 24, 2024
@alperkaya @davidsbatista
@julian-risch @LastRemote
feat: SentenceTransformersTextEmbedder supports config_kwargs (deep…
4a47446 
…set-ai#8432)

* add config_kwargs

* disable PLR0913 for a specific function

* add a release note

* refer to AutoConfig in config_kwargs docstring

---------

Co-authored-by: David S. Batista <[email protected]>
Co-authored-by: Julian Risch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julian-risch julian-risch julian-risch approved these changes

@davidsbatista davidsbatista davidsbatista approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees
No one assigned
Labels
topic:build/distribution topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exposing config_kwargs in sentence_transformers component
4 participants
@alperkaya @coveralls @julian-risch @davidsbatista