-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix extraBody loss during ModelOptionsUtils.merge() #4867
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
Conversation
The extraBody field was being lost when merging OpenAiChatOptions into ChatCompletionRequest using ModelOptionsUtils.merge(). This fix ensures that extraBody parameters (like top_k, repetition_penalty, etc.) are properly preserved during the merge operation. Added test ExtraBodyMergeTest to verify the fix. Signed-off-by: SenreySong <25841017+SenreySong@users.noreply.github.com>
|
@markpollack |
|
This is not the behavior in ser/deser that is expected for 'extra_body'. The field 'extra_body' does not go into the JSON, it get's flattened. You can see this in the openai java sdk here - and the assertion here. Also this vllm issue clarifies it more |
Thank you for clarifying the intended behavior of That said, in this Java implementation, the issue isn’t about misrepresenting in ChatCompletionRequest request = new ChatCompletionRequest(chatCompletionMessages, stream);
OpenAiChatOptions requestOptions = (OpenAiChatOptions) prompt.getOptions();
request = ModelOptionsUtils.merge(requestOptions, request, ChatCompletionRequest.class);The current I believe the fix should ensure that |
|
@markpollack By default, this merge operation only processes fields in the target class that are annotated with @JsonProperty; the extra_body field, lacking this annotation, is discarded entirely. public static <T> T merge(Object source, Object target, Class<T> clazz) {
return ModelOptionsUtils.merge(source, target, clazz, null);
}
public static <T> T merge(Object source, Object target, Class<T> clazz, List<String> acceptedFieldNames) {
if (source == null) {
source = Map.of();
}
List<String> requestFieldNames = CollectionUtils.isEmpty(acceptedFieldNames)
? REQUEST_FIELD_NAMES_PER_CLASS.computeIfAbsent(clazz, ModelOptionsUtils::getJsonPropertyValues)
: acceptedFieldNames;
if (CollectionUtils.isEmpty(requestFieldNames)) {
throw new IllegalArgumentException("No @JsonProperty fields found in the " + clazz.getName());
}
Map<String, Object> sourceMap = ModelOptionsUtils.objectToMap(source);
Map<String, Object> targetMap = ModelOptionsUtils.objectToMap(target);
targetMap.putAll(sourceMap.entrySet()
.stream()
.filter(e -> e.getValue() != null)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
targetMap = targetMap.entrySet()
.stream()
.filter(e -> requestFieldNames.contains(e.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
return ModelOptionsUtils.mapToClass(targetMap, clazz);
}``` |
|
@markpollack I encountered the same issue as @SenreySong . For the qwen3 model, model-specific parameters like enable_thinking must be passed through the extraBody mechanism, but currently extraBody is being ruthlessly discarded. Ref: https://bailian.console.aliyun.com/?tab=api#/api/?type=model&url=2712576 |
The extraBody field was being lost when merging OpenAiChatOptions into ChatCompletionRequest using ModelOptionsUtils.merge(). This fix ensures that extraBody parameters (like top_k, repetition_penalty, etc.) are properly preserved during the merge operation.
Added test ExtraBodyMergeTest to verify the fix.