- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
[ML] Custom Service add embedding type support #130141
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
[ML] Custom Service add embedding type support #130141
Conversation
…ticsearch into cs-embedding-types
| 
           Pinging @elastic/ml-core (Team:ML)  | 
    
| Map<String, Object> map, | ||
| ConfigurationParseContext context, | ||
| TaskType taskType, | ||
| String inferenceId | 
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.
inferenceId wasn't being used.
| SimilarityMeasure similarity = extractSimilarity(map, ModelConfigurations.SERVICE_SETTINGS, validationException); | ||
| Integer dims = removeAsType(map, DIMENSIONS, Integer.class); | ||
| Integer maxInputTokens = removeAsType(map, MAX_INPUT_TOKENS, Integer.class); | ||
| return new TextEmbeddingSettings(similarity, dims, maxInputTokens, DenseVectorFieldMapper.ElementType.FLOAT); | 
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 element type logic has been delegated to the TextEmbeddingResponseParser so removing it from being hard coded to float here.
| 
               | 
          ||
| if (in.getTransportVersion().before(TransportVersions.ML_INFERENCE_CUSTOM_SERVICE_EMBEDDING_TYPE) | ||
| && in.getTransportVersion().isPatchFrom(TransportVersions.ML_INFERENCE_CUSTOM_SERVICE_EMBEDDING_TYPE_8_19) == false) { | ||
| in.readOptionalEnum(DenseVectorFieldMapper.ElementType.class); | 
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.
For older versions, we'll read it but ignore it. It should only be float which we'll default to in the TextEmbeddingResponseParser.
| } | ||
| 
               | 
          ||
| protected abstract T transform(Map<String, Object> extractedField); | ||
| protected abstract InferenceServiceResults transform(Map<String, Object> extractedField); | 
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 TextEmbeddingResponseParser will now return different types of results depending on the embedding type so we need this to be the base results type to cover all them
| public static final TextEmbeddingSettings NON_TEXT_EMBEDDING_TASK_TYPE_SETTINGS = new TextEmbeddingSettings(null, null, null, null); | ||
| public static final TextEmbeddingSettings NON_TEXT_EMBEDDING_TASK_TYPE_SETTINGS = new TextEmbeddingSettings(null, null, null); | ||
| 
               | 
          ||
| public static TextEmbeddingSettings fromMap(Map<String, Object> map, TaskType taskType, ValidationException validationException) { | 
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 never included elementType in the toXContent method, so we don't have to worry about backwards compatibility with older versions of this model, correct?
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.
Yep that's correct, it was just hard coded previously.
This PR addresses the feedback item from the original PR: #127939
This PR adds support for
embedding_typeso users can specify the format of the text embeddings returned by the upstream service.Supported embedding types are:
float(default if not included)bytebit(orbinary)Comments:
#127939 (comment)
Example