Removing elastic reranker chunking feature flag and allow return_documents to be false #136045
Conversation
dan-rubinstein
added
>non-issue
:ml
dan-rubinstein
changed the title
dan-rubinstein
added
auto-backport
|
Pinging @elastic/ml-core (Team:ML) |
...a/org/elasticsearch/xpack/inference/services/elasticsearch/ElasticsearchInternalService.java
Show resolved
Hide resolved
| assertEquals(1, rankedDocResults.getRankedDocs().size()); | ||
| }, e -> fail("Expected successful parsing but got failure: " + e))); | ||
| if (returnDocuments) { | ||
| assertNotNull(rankedDocResults.getRankedDocs().get(0).text()); |
There was a problem hiding this comment.
Rather than just asserting that the text is non-null in this case, would it be better to assert that it matches the expected text:
assertThat(rankedDocResults.getRankedDocs().get(0).text(), is(inputs.getFirst()));
There was a problem hiding this comment.
This is related to the comment below. We can discuss it in the comments there.
| if (returnDocuments) { | ||
| assertNotNull(rankedDocResults.getRankedDocs().get(0).text()); | ||
| } else { | ||
| assertNull(rankedDocResults.getRankedDocs().get(0).text()); | ||
| } |
There was a problem hiding this comment.
These asserts are redundant, since we already assert that the actual results match the expected results on line 165.
There was a problem hiding this comment.
I'll remove these assertions.
There was a problem hiding this comment.
When constructing the expected RankedDoc on line 163, we're setting the text to be the expected value (either the first input or null) based on whether returnDocuments is true, so we would be able to tell if the document string was returned when doing the comparison:
new RankedDocsResults.RankedDoc(0, max(relevanceScore1, relevanceScore2), returnDocuments ? inputs.get(0) : null)
| if (returnDocuments) { | ||
| rankedDocResults.getRankedDocs().forEach(r -> { assertNotNull(r.text()); }); | ||
| } else { | ||
| rankedDocResults.getRankedDocs().forEach(r -> { assertNull(r.text()); }); | ||
| } |
There was a problem hiding this comment.
I wonder if it might be possible to assert on the actual value of the text in these tests instead of just whether or not it's null. If we used fixed values for the relevance scores in the ranked docs instead of random ones, then the order of the results would be deterministic and we could know which doc was expected to have which text.
It would also be good to have the inputs list elements not be identical, since if they're the same, then we have no way of making sure that the text is correct when comparing between the two documents. With the test as it is, if there was some weird bug which caused the text from one result to be copied to another result, we would have no way of spotting that.
There was a problem hiding this comment.
The purpose of randomizing the values of the relevance score is to make sure we always properly sort the values at the end and that we always take the highest chunk score per document. The underlying unit being tested shouldn't care whether the scores are in a specific order so I figured randomizing the values would help us test this.
As for the input strings I think we can make 2 changes to make the tests more robust.
- Make the input strings different per document to avoid accidentally generating 2 identical ones when passing in 2 documents.
- Modify the assertions to specifically check that the correct document string was returned.
Let me know if this makes sense to you.
There was a problem hiding this comment.
As long as we can assert that the results contain the correct string, then using random relevance scores is fine, I think.
Ideally, I think that a unit test should test one thing at a time, since that helps pinpoint the specific area that has a bug in the event of a test failure. For example, a test that the list returned by rankedDocResults.getRankedDocs() is sorted by relevance score regardless of the returnDocuments value would use non-random relevance scores for the inputs so that the expected output is fixed, so we don't have to effectively reimplement the sorting logic in the test in order to validate the output. This is just my personal philosophy when it comes to unit testing though, so not something that needs to be changed in this PR.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
…ments to be false (elastic#136045) * Removing elastic reranker chunking feature flag * Allow return_documents to be set to false * Updating unit tests to verify returned document strings --------- Co-authored-by: Elastic Machine <[email protected]>
💚 Backport successful
|
…ments to be false (#136045) (#136222) * Removing elastic reranker chunking feature flag * Allow return_documents to be set to false * Updating unit tests to verify returned document strings --------- Co-authored-by: Elastic Machine <[email protected]>
This change removes the feature flag for the elastic reranker chunking configuration settings. This will enable it in serverless and will remove the unnecessary code from the 9.2 branch in a backport as well.
This change also adds functionality to allow users to set
return_documentsto false in the task settings to not return document data. There is an existing bug in the ElasticsearchInternalService which still returns documents if the user setsreturn_documentsto false when creating the inference endpoint instead of when calling the perform inference API. I've created an issue to address this to minimize the size of this PR and to look into it a bit more and make sure nothing breaks if we change this behavior.