Skip to content

Conversation

@Mikep86
Copy link
Contributor

@Mikep86 Mikep86 commented Feb 27, 2025

Remove the matchedText field from chunks. This is no longer required because we build the chunk text on demand using offsets. Removing this field should help with OOMs when chunking large documents.

Copy link
Contributor

@ioanatia ioanatia left a comment

Choose a reason for hiding this comment

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

the changes make sense to me. just a couple questions.

@Mikep86
Copy link
Contributor Author

Mikep86 commented Feb 27, 2025

@elasticmachine update branch

@Mikep86 Mikep86 marked this pull request as ready for review February 27, 2025 19:19
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Feb 27, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@ioanatia ioanatia left a comment

Choose a reason for hiding this comment

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

provided the serverless failures are not related, this looks ready to me!

@Mikep86 Mikep86 added the auto-backport Automatically create backport pull requests when merged label Feb 28, 2025
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@jan-elastic
Copy link
Contributor

jan-elastic commented Mar 5, 2025

Hey @Mikep86 , I have also removed this copy of the input text as part of this PR. For the sake of preventing conflicts, I think it's more convenient to merge mine. WDYT?

It's this commit

@jan-elastic
Copy link
Contributor

On second thought: I'm also fine with merging this, and I'll rebase my PR on top of this. It's a bit of effort, but that reduces the complexity of my PR.

assertThat(chunkedFloatResult.chunks().get(3).matchedText(), startsWith(" passage_input60 "));
assertThat(chunkedFloatResult.chunks().get(4).matchedText(), startsWith(" passage_input80 "));
assertThat(chunkedFloatResult.chunks().get(5).matchedText(), startsWith(" passage_input100 "));
assertThat(chunkedFloatResult.chunks().get(0).offset(), equalTo(new ChunkedInference.TextOffset(0, 309)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I find to verify that these are the correct assertions.

I'd prefer something like:

assertThat(getMatchedText(inputs.get(1), chunkedFloatResult.chunks().get(5).offset()), startsWith(" passage_input100 "));

together with a simple helper function

private static String getMatchedText(String text, ChunkedInference.TextOffset offset) {
    return text.substring(offset.start(), offset.end());
}

Copy link
Contributor

@jan-elastic jan-elastic left a comment

Choose a reason for hiding this comment

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

LGTM, just one small comment about the test assertions

@Mikep86
Copy link
Contributor Author

Mikep86 commented Mar 5, 2025

buildkite test this

@Mikep86 Mikep86 merged commit 2fa6651 into elastic:main Mar 5, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123607

@Mikep86
Copy link
Contributor Author

Mikep86 commented Mar 5, 2025

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

Mikep86 added a commit to Mikep86/elasticsearch that referenced this pull request Mar 5, 2025
(cherry picked from commit 2fa6651)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/amazonbedrock/AmazonBedrockServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureaistudio/AzureAiStudioServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/cohere/CohereServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/openai/OpenAiServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/voyageai/VoyageAIServiceTests.java
elasticsearchmachine pushed a commit that referenced this pull request Mar 5, 2025
(cherry picked from commit 2fa6651)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/amazonbedrock/AmazonBedrockServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureaistudio/AzureAiStudioServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/cohere/CohereServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/openai/OpenAiServiceTests.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/voyageai/VoyageAIServiceTests.java
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :ml Machine learning >non-issue Team:ML Meta label for the ML team v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants