Update semantic text to use BFLOAT16 by default#144236
Update semantic text to use BFLOAT16 by default#144236Mikep86 wants to merge 28 commits intoelastic:mainfrom
Conversation
…lement type, dimensions, and similarity match what is expected
|
Hi @Mikep86, I've created a changelog YAML for you. |
|
@elasticmachine update branch |
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
|
Some tests are failing because switching to BFLOAT16 is breaking what used to be tied scores. I stabilized those tests in a separate PR: #144251 |
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Outdated
Show resolved
Hide resolved
Yep, of course we should do this.
I don't think we should require type at all? Why are we requiring it? It should be whatever default is for semantic text. If params are set that do not make sense, then of course, we should throw (e.g. setting
Yep!
yes, agreed. I think separating the index options as defined with semantic_text is OK (e.g. element_type is settable on But this might break even more backwards compatibility things. Regardless, we already need to do a "translation step" from |
server/src/main/java/org/elasticsearch/index/mapper/vectors/IndexOptions.java
Outdated
Show resolved
Hide resolved
| COSINE, | ||
| DOT_PRODUCT, | ||
| L2_NORM; | ||
| COSINE(DenseVectorFieldMapper.VectorSimilarity.COSINE), |
There was a problem hiding this comment.
please keep all refactoring that isn't strictly necessary in a separate pr.
We aren't requiring The param |
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Show resolved
Hide resolved
...ference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextIndexOptions.java
Outdated
Show resolved
Hide resolved
...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/IndexOptions.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/inference/mapper/ExtendedDenseVectorIndexOptions.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/IndexOptions.java
Outdated
Show resolved
Hide resolved
I think this is ok for now. Better to be strict here than not. I do think we COULD be lenient and just assume that |
…ce service exists fails
Updates the
semantic_textfield to use theBFLOAT16element type by default for all inference services that useFLOAT.This change minorly impacts
knnquery scoring due to the reduced precision ofBFLOAT16. This won't matter for 99% of users, but since this may be a problem for some, this PR also adds a way to override the element type used so that these users can continue usingFLOAT. Anelement_typeparameter has been added to the dense vector index options:Since
element_typeis not part of standard dense vector index options (and it doesn't make sense to add it there, sinceelement_typeis a top-level param fordense_vector), this param was added by extending dense vector index options only forsemantic_text. This creates some new scenarios for us to handle:element_typewithout settingtypetypeonly when params other thanelement_typeare setelement_typeset inindex_optionsis incompatible with the model element typeelement_typewheninclude_defaultsis true and we defaulted toBFLOAT16This PR is still in draft because tests are incomplete and there's a potentially unhandled edge case with partial defaults. Sharing now for early feedback before I invest in implementing all the tests.