Skip to content

Refactor ChunkingSettings classes from Inference plugin to XPack core #136219

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

Refactor ChunkingSettings classes from Inference plugin to XPack core #136219

wants to merge 3 commits into from

Conversation

kderusso
Copy link
Member

@kderusso kderusso commented Oct 8, 2025

This will allow chunking settings to be easily re-used by the ES|QL plugin.

kderusso
kderusso commented Oct 8, 2025
View reviewed changes
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/inference/InferenceUtils.java
Copy link
Member Author

Choose a reason for hiding this comment

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

In order to make this PR as small as possible, I surgically moved only those methods necessary from ServiceUtils and kept callers there for actual implementation messages. I did not move tests either. If I move things more aggressively this PR gets exponentially bigger.

@kderusso kderusso added >refactoring :ml/Chunking :SearchOrg/Relevance Label for the Search (solution/org) Relevance team labels Oct 9, 2025
@kderusso kderusso marked this pull request as ready for review October 9, 2025 15:38
@elasticsearchmachine elasticsearchmachine added Team:ML Meta label for the ML team Team:Search - Relevance The Search organization Search Relevance team labels Oct 9, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-relevance (Team:Search - Relevance)

DonalEvans
DonalEvans approved these changes Oct 9, 2025
View reviewed changes
Copy link
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

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

Are there any follow-up PRs planned to move tests to the appropriate module and maybe move the rest of the ServiceUtils methods too?

While moving classes can create a PR that touches a lot of files, the actual changes themselves are very easy to review, so I wouldn't mind including them as part of this PR if that would make it easier to keep everything where it should be.

@kderusso
Copy link
Member Author

kderusso commented Oct 9, 2025

@DonalEvans Sure, I can move the tests too. I didn't at first because there are still a lot of callers in ServiceUtils but agree it's cleaner.

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

@DonalEvans DonalEvans DonalEvans approved these changes

Assignees
No one assigned
Labels
:ml/Chunking >refactoring :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:ML Meta label for the ML team Team:Search - Relevance The Search organization Search Relevance team v9.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kderusso @elasticsearchmachine @DonalEvans