Skip to content

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.

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)

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.

@kderusso kderusso requested a review from DonalEvans October 10, 2025 19:50
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.

Would it be possible to also move the tests for the various chunking settings classes that were moved in this PR? Specifically to move these test classes from xpack/inference/chunking to xpack/core/inference/chunking:

  • ChunkingSettingsBuilderTests
  • NoneChunkingSettingsTests
  • RecursiveChunkingSettingsTests
  • SentenceBoundaryChunkingSettingsTests
  • WordBoundaryChunkingSettingsTests

@kderusso kderusso requested a review from DonalEvans October 13, 2025 15:53
@kderusso kderusso enabled auto-merge (squash) October 13, 2025 17:28
@kderusso kderusso merged commit 7625490 into elastic:main Oct 13, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants