-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Adding breaking change entry for retrievers #115399
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
Adding breaking change entry for retrievers #115399
Conversation
Hi @pmpailis, I've created a changelog YAML for you. Note that since this PR is labelled |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
docs/changelog/115399.yaml
Outdated
To ensure consistency and avoid unexpected failures, we have introduced a new cluster_feature that | ||
all nodes must satisfy, in order for the `rrf` and `text_similarity_reranker` retriever queries to be evaluated. | ||
This guarantees that all nodes are able to take advantage of the new framework and provide compatible responses. | ||
As part of the rework, we have also removed the `_rank` property from the responses of an `rrf` retriever. |
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.
Don't indicate cluster_feature
or anything. Simply indicate the behavior. Don't reference internal details.
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 Ben, that's fair! Reworded it slightly, lmtk what you think :)
docs/changelog/115399.yaml
Outdated
To ensure consistency and avoid unexpected failures, `rrf` and `text_similarity_reranker` retriever queries are no longer compatible | ||
with previous releases ( <= 8.15). |
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.
YIKES, this implies that rrf
and text_similarity_reranker
no longer works! Please make sure you are very clear that they only break in a mixed cluster scenario, not that they stop working in 8.16 :)
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.
hahah that's definitely not what I was going for 😅 had the mixed cluster description in the impact
section below,
so I thought to not include it here as well.
WDYT about the following?
In this release (8.16), we have introduced major changes to the retrievers framework
and how they can be evaluated, focusing mainly on compound retrievers
like `rrf` and `text_similarity_reranker`, which allowed us to support full
composability (i.e. any retriever can be nested under any compound retriever),
as well as supporting additional search features like collapsing, explaining,
aggregations, and highlighting.
To ensure consistency, and given that this rework is not available until 8.16,
`rrf` and `text_similarity_reranker` retriever queries would now
throw an exception in a mixed cluster scenario, where there are nodes
both in current or later (i.e. >= 8.16) and previous ( <= 8.15) versions.
As part of the rework, we have also removed the `_rank` property from
the responses of an `rrf` retriever.
@elasticmachine update branch |
Co-authored-by: Elastic Machine <[email protected]>
Friendly reminder that this PR seems to be a breaking change for 9.0, but is missing from the 9.0 release note. We may want to add an entry to the breaking change section of 9.0 release note. |
Updating exception thrown when cluster_feature is not present for
rrf
andtext_similarity_reranker
and adding breaking change entry for the work taken place in 8.16 (#112648)