-
Notifications
You must be signed in to change notification settings - Fork 26.1k
[Inference API] Add custom headers for Azure OpenAI Service #142969
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
Changes from 8 commits
f2a21e9
ebdb4a6
bb0907e
8cedcd0
c4b846d
e3d466f
bc2384e
e7a8fc2
56bf6dc
1f64d15
4741068
f1e92f7
e55d554
2eb28ce
838ede1
6242017
bfd1953
3313304
e13ba77
4ae979a
569673e
f03baf4
2d9f471
efb0956
7cb22c3
0b1cff0
80a2272
2b600d2
8ebbd3b
710f8aa
83df8d5
179d7b2
30d256d
fe979eb
c75b7f4
e24b927
3fe6c9e
69b4732
ef2b448
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| area: Inference | ||
| issues: [] | ||
| pr: 142969 | ||
| summary: "[Inference API] Add custom headers for Azure OpenAI Service" | ||
| type: enhancement |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 9294000 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| shard_heap_usage_in_cluster_info,9293000 | ||
| inference_azure_openai_task_settings_headers,9294000 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.inference.common.parser; | ||
|
|
||
| import org.elasticsearch.common.ValidationException; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.common.io.stream.Writeable; | ||
| import org.elasticsearch.xcontent.ConstructingObjectParser; | ||
| import org.elasticsearch.xcontent.ParseField; | ||
| import org.elasticsearch.xcontent.ToXContentFragment; | ||
| import org.elasticsearch.xcontent.XContentBuilder; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg; | ||
| import static org.elasticsearch.xpack.inference.services.ServiceUtils.removeNullValues; | ||
| import static org.elasticsearch.xpack.inference.services.ServiceUtils.validateMapStringValues; | ||
|
|
||
| public record Headers(Map<String, String> headersMap) implements ToXContentFragment, Writeable { | ||
|
|
||
| private static final ParseField HEADERS = new ParseField("headers"); | ||
|
|
||
| public static final Headers EMPTY_INSTANCE = new Headers(Map.of()); | ||
|
|
||
| public static <Value, Context> void initParser(ConstructingObjectParser<Value, Context> parser) { | ||
| parser.declareObjectOrNull(optionalConstructorArg(), (p, c) -> p.mapOrdered(), null, HEADERS); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public static Headers create(Object arg) { | ||
| if (arg == null) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+69
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an argument for returning EMPTY_INSTANCE here? I think it would allow us to make the headers field on AzureOpenAiTaskSettings not @nullable, so we'd have to check .isEmpty() instead of null in a few places, but it would mean that we wouldn't end up potentially creating a AzureOpenAiTaskSettings with a null user and empty headers, which could happen at the moment. Alternately, we could check if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, now that I'm thinking about this more I wonder how a user would use the If we removed I think we can use I agree though, it'd be nice to have a single state with only empty headers. Let me know if you can think of a way to handle that. I suppose we could use an enum as well 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok give this another look, I think it's in a better state 😅 |
||
|
|
||
| var validationException = new ValidationException(); | ||
|
|
||
| removeNullValues((Map<String, Object>) arg); | ||
|
|
||
| var stringHeaders = validateMapStringValues( | ||
| (Map<String, String>) arg, | ||
| HEADERS.getPreferredName(), | ||
| validationException, | ||
| false, | ||
| Map.of() | ||
| ); | ||
|
|
||
| if (validationException.validationErrors().isEmpty() == false) { | ||
| throw validationException; | ||
| } | ||
|
|
||
| if (stringHeaders.isEmpty()) { | ||
| return EMPTY_INSTANCE; | ||
| } | ||
|
|
||
| return new Headers(stringHeaders); | ||
| } | ||
|
|
||
| public Headers { | ||
| Objects.requireNonNull(headersMap, "headers map is required"); | ||
| } | ||
|
|
||
| public Headers(StreamInput in) throws IOException { | ||
| this(in.readImmutableMap(StreamInput::readString, StreamInput::readString)); | ||
| } | ||
|
|
||
| public boolean isEmpty() { | ||
| return headersMap.isEmpty(); | ||
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field(HEADERS.getPreferredName(), headersMap); | ||
| return builder; | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeMap(headersMap, StreamOutput::writeString, StreamOutput::writeString); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ | |
| import org.elasticsearch.common.ValidationException; | ||
| import org.elasticsearch.core.Nullable; | ||
| import org.elasticsearch.inference.ModelConfigurations; | ||
| import org.elasticsearch.xpack.inference.services.azureopenai.embeddings.AzureOpenAiEmbeddingsRequestTaskSettings; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
|
|
@@ -31,7 +30,7 @@ public record AzureAiStudioEmbeddingsRequestTaskSettings(@Nullable String user) | |
| * does not throw an error. | ||
| * | ||
| * @param map the settings received from a request | ||
| * @return a {@link AzureOpenAiEmbeddingsRequestTaskSettings} | ||
| * @return a {@link AzureAiStudioEmbeddingsRequestTaskSettings} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was referencing the wrong class |
||
| */ | ||
| public static AzureAiStudioEmbeddingsRequestTaskSettings fromMap(Map<String, Object> map) { | ||
| if (map.isEmpty()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,222 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.inference.services.azureopenai; | ||
|
|
||
| import org.elasticsearch.TransportVersion; | ||
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.common.ValidationException; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.core.Nullable; | ||
| import org.elasticsearch.inference.ModelConfigurations; | ||
| import org.elasticsearch.inference.TaskSettings; | ||
| import org.elasticsearch.xcontent.ConstructingObjectParser; | ||
| import org.elasticsearch.xcontent.ParseField; | ||
| import org.elasticsearch.xcontent.XContentBuilder; | ||
| import org.elasticsearch.xcontent.XContentParserConfiguration; | ||
| import org.elasticsearch.xcontent.json.JsonXContent; | ||
| import org.elasticsearch.xpack.core.inference.InferenceUtils; | ||
| import org.elasticsearch.xpack.inference.common.parser.Headers; | ||
| import org.elasticsearch.xpack.inference.services.ConfigurationParseContext; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg; | ||
|
|
||
| /** | ||
| * Base class for Azure OpenAI task settings (embeddings and completion). Holds optional user and optional | ||
| * custom HTTP headers via {@link Headers}. | ||
| */ | ||
| public abstract class AzureOpenAiTaskSettings<T extends AzureOpenAiTaskSettings<T>> implements TaskSettings { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This generally follows the same pattern that we started in OpenAiTaskSettings except this class leverages a ConstructingObjectParser to parse out the fields. Since we'd like to move in that direction, hopefully this will make that easier in the future. |
||
|
|
||
| private static final Settings EMPTY_SETTINGS = new Settings(null, null); | ||
|
|
||
| protected static final TransportVersion INFERENCE_AZURE_OPENAI_TASK_SETTINGS_HEADERS = TransportVersion.fromName( | ||
| "inference_azure_openai_task_settings_headers" | ||
| ); | ||
|
|
||
| protected record Settings(@Nullable String user, @Nullable Headers headers) {} | ||
|
|
||
| private static final ConstructingObjectParser<Settings, Void> STORAGE_PARSER = createParser(true); | ||
| private static final ConstructingObjectParser<Settings, Void> REQUEST_PARSER = createParser(false); | ||
|
|
||
| private static ConstructingObjectParser<Settings, Void> createParser(boolean ignoreUnknownFields) { | ||
| ConstructingObjectParser<Settings, Void> constructingObjectParser = new ConstructingObjectParser<>( | ||
| "azure_openai_task_settings_parser", | ||
| ignoreUnknownFields, | ||
| args -> createSettings((String) args[0], Headers.create(args[1])) | ||
| ); | ||
|
|
||
| constructingObjectParser.declareString(optionalConstructorArg(), new ParseField(AzureOpenAiServiceFields.USER)); | ||
| Headers.initParser(constructingObjectParser); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apply the header parsing logic to the constructing object parser.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is where we would call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I was struggling with that. The issue I see is that headers is only a map so there isn't another level for parsing. For example if we had this: Then the headers class could have a parser that explicitly looks for In the examples I've seen we typically declare another parser if it encapsulates a whole object.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes, of course. Makes sense 👍 |
||
|
|
||
| return constructingObjectParser; | ||
| } | ||
|
|
||
| private static Settings createSettings(@Nullable String user, @Nullable Headers headers) { | ||
| if (user == null && headers == null) { | ||
| return EMPTY_SETTINGS; | ||
| } | ||
| return new Settings(user, headers); | ||
| } | ||
|
|
||
| protected abstract static class Factory<T> { | ||
| private T emptyInstance; | ||
|
|
||
| protected abstract T create(@Nullable String user, @Nullable Headers headers); | ||
|
|
||
| protected abstract T createEmptyInstance(); | ||
|
|
||
| public T emptySettings() { | ||
| // Ideally we'd be able to pass the empty instance in via the Factory constructor, but since the empty instance relies on the | ||
| // factory to be created, we have to lazily create it here. The empty instance will call the AzureOpenAiTaskSettings | ||
| // constructor with the factory. If we don't do it this way we end up getting an NPE in the constructor because the factory | ||
| // hasn't finished initialization yet. | ||
| if (emptyInstance == null) { | ||
| emptyInstance = createEmptyInstance(); | ||
| } | ||
| return emptyInstance; | ||
| } | ||
| } | ||
|
|
||
| protected static <T extends AzureOpenAiTaskSettings<T>> T parseSettingsFromMap( | ||
| Map<String, Object> map, | ||
| ConfigurationParseContext configurationParseContext, | ||
| Factory<T> factory | ||
| ) { | ||
| if (map.isEmpty()) { | ||
| return factory.emptySettings(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another option I looked into was to use an abstract method on the
|
||
| } | ||
|
|
||
| try { | ||
| try ( | ||
| var xContent = XContentBuilder.builder(JsonXContent.jsonXContent).map(map); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This converts a Map to json and then parses it. |
||
| var parser = JsonXContent.jsonXContent.createParser(XContentParserConfiguration.EMPTY, Strings.toString(xContent)) | ||
| ) { | ||
| Settings createdSettings; | ||
|
|
||
| if (configurationParseContext == ConfigurationParseContext.REQUEST) { | ||
| createdSettings = REQUEST_PARSER.parse(parser, null); | ||
| validateSettings(createdSettings); | ||
| } else { | ||
| createdSettings = STORAGE_PARSER.parse(parser, null); | ||
| } | ||
|
|
||
| return factory.create(createdSettings.user(), createdSettings.headers()); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new IllegalArgumentException("Failed to parse Azure OpenAI task settings", e); | ||
| } | ||
| } | ||
|
|
||
| private static void validateSettings(Settings settings) { | ||
| var validationException = new ValidationException(); | ||
|
|
||
| if (settings.user() != null && settings.user().isEmpty()) { | ||
| validationException.addValidationError( | ||
| InferenceUtils.mustBeNonEmptyString(AzureOpenAiServiceFields.USER, ModelConfigurations.TASK_SETTINGS) | ||
| ); | ||
| throw validationException; | ||
| } | ||
| } | ||
|
|
||
| private final Settings taskSettings; | ||
| private final Factory<T> factory; | ||
|
|
||
| protected AzureOpenAiTaskSettings(@Nullable String user, @Nullable Headers headers, Factory<T> factory) { | ||
| this(createSettings(user, headers), factory); | ||
| } | ||
|
|
||
| protected AzureOpenAiTaskSettings(Settings taskSettings, Factory<T> factory) { | ||
| this.taskSettings = Objects.requireNonNull(taskSettings); | ||
| this.factory = Objects.requireNonNull(factory); | ||
| } | ||
|
|
||
| protected AzureOpenAiTaskSettings(StreamInput in, Factory<T> factory) throws IOException { | ||
| this(readTaskSettingsFromStream(in), factory); | ||
| } | ||
|
|
||
| private static Settings readTaskSettingsFromStream(StreamInput in) throws IOException { | ||
| var user = in.readOptionalString(); | ||
| var headers = in.getTransportVersion().supports(INFERENCE_AZURE_OPENAI_TASK_SETTINGS_HEADERS) | ||
| ? in.readOptionalWriteable(Headers::new) | ||
| : null; | ||
| return createSettings(user, headers); | ||
| } | ||
|
|
||
| public String user() { | ||
| return taskSettings.user(); | ||
| } | ||
|
|
||
| public Headers headers() { | ||
| return taskSettings.headers(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| var user = taskSettings.user(); | ||
| var headers = taskSettings.headers(); | ||
| return (user == null || user.isEmpty()) && (headers == null || headers.isEmpty()); | ||
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.startObject(); | ||
| if (taskSettings.user() != null) { | ||
| builder.field(AzureOpenAiServiceFields.USER, taskSettings.user()); | ||
| } | ||
| if (taskSettings.headers() != null) { | ||
| taskSettings.headers().toXContent(builder, params); | ||
| } | ||
|
Comment on lines
+213
to
215
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might cause problems, since we parse the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't really be possible to get in a scenario where
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, that should be fine then |
||
| builder.endObject(); | ||
| return builder; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| AzureOpenAiTaskSettings<?> that = (AzureOpenAiTaskSettings<?>) o; | ||
| return Objects.equals(taskSettings, that.taskSettings); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(taskSettings); | ||
| } | ||
|
|
||
| @Override | ||
| public T updatedTaskSettings(Map<String, Object> newSettings) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't try to return an empty instance. To return an empty instance we'd need to cast to If you have other ideas I'm open to improving this though.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with this as it is, but one option would be to have an abstract at the end of this method.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, we actually have this via the |
||
| var updated = parseSettingsFromMap(new HashMap<>(newSettings), ConfigurationParseContext.REQUEST, factory); | ||
| var userToUse = updated.user() == null ? taskSettings.user() : updated.user(); | ||
| var headersToUse = updated.headers() == null ? taskSettings.headers() : updated.headers(); | ||
| return factory.create(userToUse, headersToUse); | ||
| } | ||
|
|
||
| @Override | ||
| public TransportVersion getMinimalSupportedVersion() { | ||
| assert false : "should never be called when supportsVersion is used"; | ||
| return INFERENCE_AZURE_OPENAI_TASK_SETTINGS_HEADERS; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean supportsVersion(TransportVersion version) { | ||
| return INFERENCE_AZURE_OPENAI_TASK_SETTINGS_HEADERS.supports(version); | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeOptionalString(user()); | ||
| if (out.getTransportVersion().supports(INFERENCE_AZURE_OPENAI_TASK_SETTINGS_HEADERS)) { | ||
| out.writeOptionalWriteable(headers()); | ||
| } | ||
| } | ||
| } | ||
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.
Should the
nullValueargument here beMap.of()instead ofnull? If you set"headers": nullin the JSON, we get an NPE right now.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 switch it and added a test 👍
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.
Should we still provide
Map.of()as the null value?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.
Seeing the comment right after, there is also a
p.mapStrings()that parses the field asMap<String, String>. See an example usage of that inDatafeedConfigwhich also has headers.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 typical way of doing this would be to have a static parser built here. Then, instead of calling
initParser, we can calldeclareObject(...)and pass the parser in as argument.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.
Let's sync on this tomorrow 👍
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.
I also added a sentinel value so we can identify when the user explicitly sets the value to
null.