-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Check for null before overriding task settings #102918
Conversation
Pinging @elastic/ml-core (Team:ML) |
@@ -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()) { |
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 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?
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.
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" : {
}
public void testOverrideWith_NullOrEmptyMap() { | ||
var model = createModel("url", "org", "api_key", "model_name", null); | ||
|
||
var requestTaskSettingsMap = randomBoolean() ? null : Map.<String, Object>of(); |
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 think this kind of randomization is considered bad practice, see: https://github.com/elastic/elasticsearch/blob/main/TESTING.asciidoc#bad-practices
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 wanted to ensure that a specific code path was taken and was less concerned about which clause in the conditional test was triggered. I created separate test cases for each to be more explicit and the naming of the tests also aids understandability.
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.
LGTM
Non issue as this fixes a bug in unreleased code