Skip to content
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.

Sign up for GitHub

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

Check for null before overriding task settings #102918

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Check for null before overriding task settings #102918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e61166
Check for null task settings
davidkyle Dec 2, 2023
8ac7f7a
Add test case
davidkyle Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...a/org/elasticsearch/xpack/inference/services/openai/embeddings/OpenAiEmbeddingsModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,11 @@ public ExecutableAction accept(OpenAiActionVisitor creator, Map<String, Object>
}

public OpenAiEmbeddingsModel overrideWith(Map<String, Object> taskSettings) {
var requestTaskSettings = OpenAiEmbeddingsRequestTaskSettings.fromMap(taskSettings);
if (taskSettings == null || taskSettings.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this behavior.

If I call model.overrideWith(Map.of()), I expect the settings to be cleared, not to be preserved. Why this unintuitiuve behavior?

Copy link
Member Author

@davidkyle davidkyle Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is hit when the inference API is called, the inference endpoint allows the user to override the default model task settings either for experimentation or to change the behaviour in certain situations

POST _inference/text_embedding/my_model 
{
  input: ["....],
  task_settings: {
    "temperature": 5
  }
}

For an setting to be overridden it must be explicitly set.

"task_settings" : {
  "temperature": 5
}

Calling the API with empty task_settings does not explicitly overwrite any options, in this case the user probably didn't mean wipe all the task settings, what does it mean to wipe the defaults anyway as what would we replace them with.

"task_settings" : {
}

return this;
}

var requestTaskSettings = OpenAiEmbeddingsRequestTaskSettings.fromMap(taskSettings);
return new OpenAiEmbeddingsModel(this, getTaskSettings().overrideWith(requestTaskSettings));
}
}
19 changes: 19 additions & 0 deletions .../elasticsearch/xpack/inference/services/openai/embeddings/OpenAiEmbeddingsModelTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@
import org.elasticsearch.xpack.inference.services.openai.OpenAiServiceSettings;
import org.elasticsearch.xpack.inference.services.settings.DefaultSecretSettings;

import java.util.Map;

import static org.elasticsearch.xpack.inference.services.openai.embeddings.OpenAiEmbeddingsRequestTaskSettingsTests.getRequestTaskSettingsMap;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.sameInstance;

public class OpenAiEmbeddingsModelTests extends ESTestCase {

Expand All @@ -28,6 +31,22 @@ public void testOverrideWith_OverridesUser() {
assertThat(overriddenModel, is(createModel("url", "org", "api_key", "model_name", "user_override")));
}

public void testOverrideWith_EmptyMap() {
var model = createModel("url", "org", "api_key", "model_name", null);

var requestTaskSettingsMap = Map.<String, Object>of();

var overriddenModel = model.overrideWith(requestTaskSettingsMap);
assertThat(overriddenModel, sameInstance(model));
}

public void testOverrideWith_NullMap() {
var model = createModel("url", "org", "api_key", "model_name", null);

var overriddenModel = model.overrideWith(null);
assertThat(overriddenModel, sameInstance(model));
}

public static OpenAiEmbeddingsModel createModel(
String url,
@Nullable String org,
Expand Down