-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Fixing randomization issue in random rank retriever yaml tests #114877
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.
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
Fixing randomization issue in random rank retriever yaml tests #114877
Conversation
Pinging @elastic/search-eng (Team:SearchOrg) |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
settings: | ||
number_of_shards: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmpailis just so you know, it is possible in the CCS test cases that you end up with effectively 2 shards
which has one remote and one local.
What makes this sensitive to shard count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue is with the idf component of the query_string
. Looking into explain
for the first doc we have:
- 2 shards case:
{
"value": 0.13353139,
"description": "idf, computed as log(1 + (N - n + 0.5) / (n + 0.5)) from:",
"details":
[
{
"value": 3,
"description": "n, number of documents containing term",
"details":
[]
},
{
"value": 3,
"description": "N, total number of documents with field",
"details":
[]
}
]
},
1 shard case:
{
"value": 0.087011375,
"description": "idf, computed as log(1 + (N - n + 0.5) / (n + 0.5)) from:",
"details":
[
{
"value": 5,
"description": "n, number of documents containing term",
"details":
[]
},
{
"value": 5,
"description": "N, total number of documents with field",
"details":
[]
}
]
},
Which is why the final score computed at the end is not the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this. My suggestion is to merge this fix, and if we believe there are improvements we can make to this type of testing strategy due to CCS apply them in a followup PR.
It seems that the expected scores specified in the yaml test match when we have only 1 shard. If due to randomization we may end up with more than one shards (e.g. when using
-Dtests.seed=1449F1BCAD7A1D83
) then the scores differ and the test fails. So, in this PR we just ensure that we have only 1 shard in all cases.Closes #111999