-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Default new semantic_text fields to use BBQ when models are compatible #126629
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
Merged
kderusso
merged 14 commits into
elastic:main
from
kderusso:kderusso/semantic-text-default-bbq
Apr 17, 2025
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
d4ddcd0
Default new semantic_text fields to use BBQ when models are compatible
kderusso add2b72
Update docs/changelog/126629.yaml
kderusso 071a7bd
Merge branch 'main' into kderusso/semantic-text-default-bbq
kderusso 589f657
Gate default BBQ by IndexVersion
kderusso 10f3a86
Cleanup from PR feedback
kderusso 6bc674c
PR feedback
kderusso b318ffe
Fix test
kderusso 6cfe39a
Fix test
kderusso be9b7b4
Merge from main
kderusso 998b3b9
PR feedback
kderusso 1460038
Update test to test correct options
kderusso 4a7fad9
Hack alert: Fix issue where mapper service was always being created w…
kderusso 06cf079
Merge branch 'main' into kderusso/semantic-text-default-bbq
kderusso 7742bfc
Merge branch 'main' into kderusso/semantic-text-default-bbq
kderusso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 126629 | ||
summary: Default new `semantic_text` fields to use BBQ when models are compatible | ||
area: Relevance | ||
type: enhancement | ||
issues: [] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Based on your changes in
SemanticTextFieldMapperTests
, I don't think we need any changes togetRandomCompatibleIndexVersion
? These changes create some usability challenges:maxVersion
is only selectively used, which could be confusing to future devs.maxVersion
is less thanIndexVersions.UPGRADE_TO_LUCENE_10_0_0
orIndexVersions.INFERENCE_METADATA_FIELDS
? This will result in an error, and one that is thrown based on a randomization factor as well, which will be very confusing.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.
We need this because otherwise one of the cases in
SemanticInferenceMetadataFieldMapperTests#isEnabled
will fail as it depends on the old, less inclusive legacy format logic. You can see the modifications to the test for this here: 6cfe39aI could have removed the test, but that didn't feel right. I think this is OK, if we need to extend it later for additional tests we can do so as needed.
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.
What I'm confused about is why reverting changes to both
getRandomCompatibleIndexVersion
andtestIsEnabled
isn't an option. This should work fine as this is the state of the test before this PR, no?And that approach should work with your changes in
SemanticTextFieldMapperTests
, so I see it as the best path for the moment.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.
We already have some followup work on this test, why don't we roll that into the scope?