Skip to content

Extend huggingface with rerank #127297

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
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Extend huggingface with rerank #127297

wants to merge 5 commits into from
+848 −18

Conversation

Evgenii-Kazannik
Copy link

@Evgenii-Kazannik Evgenii-Kazannik commented Apr 23, 2025 •
edited
Loading

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 23, 2025
jonathan-buttner
jonathan-buttner reviewed Apr 24, 2025
View reviewed changes
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferencePlugin.java Outdated
@@ -361,6 +362,7 @@ public void loadExtensions(ExtensionLoader loader) {
public List<InferenceServiceExtension.Factory> getInferenceServiceFactories() {
return List.of(
context -> new HuggingFaceElserService(httpFactory.get(), serviceComponents.get()),
context -> new HuggingFaceRerankService(httpFactory.get(), serviceComponents.get()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a whole new service we'll want to add the functionality to the HuggingFaceService.

For example the OpenAI service supports many task types: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/openai/OpenAiService.java#L175-L197

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, Jonathan. Now the HuggingFaceService is used

jonathan-buttner
jonathan-buttner reviewed Apr 24, 2025
View reviewed changes
.../org/elasticsearch/xpack/inference/services/huggingface/HuggingFaceRequestRerankManager.java Outdated

import static org.elasticsearch.xpack.inference.common.Truncator.truncate;

public class HuggingFaceRequestRerankManager extends BaseRequestManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to move away from the request manager pattern because it adds duplicate code. Could you look into following the pattern we started here (we haven't refactored all the services yet but if it's possible to do for hugging face it'd be great if we could do it now)?

#124144

One option would be to leave the other hugging face request managers as they are (if possible, it may not be though) and then use one of the generic request managers like shown in the PR above for this new functionality.

We'll want to sync up with @Jan-Kazlouski-elastic to ensure that we're not duplicating that refactoring work though.

Copy link
Author

Choose a reason for hiding this comment

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

We synced with Jan. I followed the changes he applied with GenericRequestManager

Jan-Kazlouski-elastic
Jan-Kazlouski-elastic reviewed Apr 25, 2025
View reviewed changes
...sticsearch/xpack/inference/services/huggingface/rerank/HuggingFaceRerankServiceSettings.java

@Override
public String modelId() {
return null;
Copy link

Choose a reason for hiding this comment

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

I would be adding comments for such implementations.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Added the comment

@gbanasiak gbanasiak added the :ml Machine learning label Apr 28, 2025
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Apr 28, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Apr 28, 2025
@Evgenii-Kazannik Evgenii-Kazannik force-pushed the extend-huggingface-with-rerank branch from 586ae7f to 390b7e7 Compare April 28, 2025 07:59
@Evgenii-Kazannik Evgenii-Kazannik marked this pull request as draft April 28, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-buttner jonathan-buttner jonathan-buttner left review comments

@Jan-Kazlouski-elastic Jan-Kazlouski-elastic Jan-Kazlouski-elastic left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team :ml Machine learning Team:ML Meta label for the ML team v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Evgenii-Kazannik @elasticsearchmachine @jonathan-buttner @Jan-Kazlouski-elastic @gbanasiak