Skip to content

Commit fbd7ae7

Browse files
atobiszeiatobiszei
committed
Review fixes
1 parent 9b8c2a3 commit fbd7ae7

File tree

4 files changed

+45
-35
lines changed

4 files changed

+45
-35
lines changed

‎src/llm/apis/openai_completions.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1061,7 +1061,6 @@ std::string OpenAIChatCompletionsHandler::serializeStreamingChunk(const std::str
10611061
choice.AddMember("logprobs", Value(), allocator);
10621062
if (endpoint == Endpoint::CHAT_COMPLETIONS) {
10631063
if (outputParser != nullptr) {
1064-
// FIXME need tool maps for streaming
10651064
std::optional<Document> delta = outputParser->parseChunk(chunkResponse, areToolsAvailable(), finishReason);
10661065
if (!delta.has_value()) {
10671066
return "";

‎src/llm/io_processing/output_parser.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ std::optional<rapidjson::Document> OutputParser::parseChunk(const std::string& c
232232
*/
233233

234234
bool reasoningParserExistsAndSupportsStreaming = reasoningParser && !reasoningParser->getParsingStartTag().empty() && !reasoningParser->getParsingEndTag().empty();
235-
bool toolParserExistsAndSupportsStreaming = toolParser && !toolParser->getParsingStartTag().empty(); // FIXME why not check for parsingEntTag not empty?
235+
bool toolParserExistsAndSupportsStreaming = toolParser && !toolParser->getParsingStartTag().empty();
236236
bool applyToolParser = toolParserExistsAndSupportsStreaming && toolsAvailable;
237237

238238
if (applyToolParser && toolParser->isImmediateParsingEnabled() && processingPhase == UNKNOWN) {

‎src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,9 +319,8 @@ std::optional<rapidjson::Document> Qwen3CoderToolParser::sendFullDelta(std::opti
319319
auto& toolCalls = toolCallsOpt.value();
320320
if (toolCalls.size() != 1) {
321321
SPDLOG_ERROR("For streaming we expected one tool call, got: {}", toolCalls.size());
322-
}
323-
if (toolCalls.size() < 1) {
324-
return std::nullopt;
322+
// TODO we should return status code but this require change of parsers API
323+
throw std::runtime_error("For streaming we expected one tool call");
325324
}
326325
auto& toolCall = toolCalls[0];
327326
rapidjson::Document argsDelta;

‎src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp‎

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -38,27 +38,28 @@ using ovms::ToolsParameterTypeMap_t;
3838
static std::unique_ptr<ov::genai::Tokenizer> qwen3Tokenizer;
3939

4040
static std::map<std::string, std::string> toolSchemasInput = {
41-
{"string_tool", R"({"properties": {"arg1": {"type": "string", "description": "A string argument."}}, "required": ["arg1"]})"}};
42-
static rapidjson::Document testSchemasDoc;
41+
{"string_tool", R"({"properties": {"arg1": {"type": "string", "description": "A string argument."}}, "required": ["arg1"]})"},
42+
{"string_int_tool", R"({"properties":{"arg1":{"type":"string","description":"A string argument."},"arg2":{"type":"integer","description":"An integer argument."}},"required":["arg1", "arg2"]})"},
43+
{"some_tool", R"({"properties":{"source":{"type":"string","description":"The name of the file or directory to copy."},"destination":{"type":"string","description":"The destination name to copy the file or directory to. If the destination is a directory, the source will be copied into this directory. No file paths allowed. "}},"required":[]})"}};
44+
45+
static std::vector<std::unique_ptr<rapidjson::Document>> schemaDocsStorage;
4346

4447
static ToolsSchemas_t convertStringToolSchemasStringToToolsSchemas(
45-
const std::map<std::string, std::string>& input,
46-
rapidjson::Document& doc) {
48+
const std::map<std::string, std::string>& input) {
4749
ToolsSchemas_t result;
48-
auto& allocator = doc.GetAllocator();
50+
schemaDocsStorage.clear();
4951
for (const auto& [name, schemaStr] : input) {
50-
rapidjson::Document schemaDoc;
51-
if (schemaDoc.Parse(schemaStr.c_str()).HasParseError()) {
52+
auto schemaDoc = std::make_unique<rapidjson::Document>();
53+
if (schemaDoc->Parse(schemaStr.c_str()).HasParseError()) {
5254
throw std::runtime_error("Failed to parse schema for tool: " + name);
5355
}
54-
rapidjson::Value schemaCopy(schemaDoc, allocator);
55-
doc.CopyFrom(schemaCopy, allocator);
56-
result[name] = {&doc, schemaStr};
56+
result[name] = {schemaDoc.get(), schemaStr};
57+
schemaDocsStorage.push_back(std::move(schemaDoc));
5758
}
58-
5959
return result;
6060
}
61-
static ovms::ToolsSchemas_t toolsSchemas = convertStringToolSchemasStringToToolsSchemas(toolSchemasInput, testSchemasDoc);
61+
62+
static ovms::ToolsSchemas_t toolsSchemas = convertStringToolSchemasStringToToolsSchemas(toolSchemasInput);
6263
static ToolsParameterTypeMap_t toolsParametersTypeMap = {
6364
{"string_tool", {{"arg1", ParameterType::STRING}}},
6465
{"string_string_tool", {{"arg1", ParameterType::STRING}, {"arg2", ParameterType::STRING}}},
@@ -88,26 +89,25 @@ class Qwen3CoderOutputParserTest : public ::testing::Test {
8889
}
8990

9091
void SetUp() override {
91-
// For Qwen3 model we use hermes3 tool parser (due to the same format of generated tool calls) and qwen3 reasoning parser
9292
outputParser = std::make_unique<OutputParser>(*qwen3Tokenizer, "qwen3coder", "", toolsSchemas);
9393
}
94-
std::tuple<ov::Tensor, std::vector<int64_t>, ParsedOutput> doTheWork(const std::string& input) {
94+
std::tuple<ov::Tensor, std::vector<int64_t>, ParsedOutput> generateParsedOutput(const std::string& input) {
9595
auto generatedTensor = qwen3Tokenizer->encode(input, ov::genai::add_special_tokens(false)).input_ids;
9696
std::vector<int64_t> generatedTokens(generatedTensor.data<int64_t>(), generatedTensor.data<int64_t>() + generatedTensor.get_size());
9797
ParsedOutput parsedOutput = outputParser->parse(generatedTokens, true);
9898
return {generatedTensor, generatedTokens, parsedOutput};
9999
}
100100
};
101101
TEST_F(Qwen3CoderOutputParserTest, Parse1ToolCall1Function1ArgumentTagsNewline) {
102-
std::string input = R"(io_processing/hermes3/generation_config_builder.cpp
102+
std::string input = R"(
103103
"<tool_call>
104104
<function=string_tool>
105105
<parameter=arg1>
106106
value1
107107
</parameter>
108108
</function>
109109
</tool_call>")";
110-
auto [generatedTensor, generatedTokens, parsedOutput] = doTheWork(input);
110+
auto [generatedTensor, generatedTokens, parsedOutput] = generateParsedOutput(input);
111111

112112
ASSERT_EQ(parsedOutput.toolCalls.size(), 1);
113113
EXPECT_EQ(parsedOutput.toolCalls[0].name, "string_tool");
@@ -124,18 +124,31 @@ TEST_F(Qwen3CoderOutputParserTest, Parse1ToolCallNestedXmlNotFromSchema) {
124124
</parameter>
125125
</function>
126126
</tool_call>")";
127-
auto [generatedTensor, generatedTokens, parsedOutput] = doTheWork(input);
127+
auto [generatedTensor, generatedTokens, parsedOutput] = generateParsedOutput(input);
128128

129129
ASSERT_EQ(parsedOutput.toolCalls.size(), 1);
130130
EXPECT_EQ(parsedOutput.toolCalls[0].name, "string_tool");
131131
EXPECT_EQ(parsedOutput.toolCalls[0].arguments, "{\"arg1\": \"<value=abc>value1</value>\"}");
132132
EXPECT_EQ(parsedOutput.toolCalls[0].id.empty(), false);
133133
}
134-
// FIXME check if two tool calls is a vali for outputparser as well not only for parser imple
134+
TEST_F(Qwen3CoderOutputParserTest, ParseTwoToolCalls1Function1ArgumentTagsNoNewline) {
135+
std::string input = R"(
136+
"<tool_call><function=string_tool><parameter=arg1>value1</parameter></function></tool_call>"
137+
"<tool_call><function=string_tool><parameter=arg1>value2</parameter></function></tool_call>")";
138+
auto [generatedTensor, generatedTokens, parsedOutput] = generateParsedOutput(input);
139+
140+
ASSERT_EQ(parsedOutput.toolCalls.size(), 2);
141+
EXPECT_EQ(parsedOutput.toolCalls[0].name, "string_tool");
142+
EXPECT_EQ(parsedOutput.toolCalls[0].arguments, "{\"arg1\": \"value1\"}");
143+
EXPECT_EQ(parsedOutput.toolCalls[0].id.empty(), false);
144+
EXPECT_EQ(parsedOutput.toolCalls[1].name, "string_tool");
145+
EXPECT_EQ(parsedOutput.toolCalls[1].arguments, "{\"arg1\": \"value2\"}");
146+
EXPECT_EQ(parsedOutput.toolCalls[1].id.empty(), false);
147+
}
135148
TEST_F(Qwen3CoderOutputParserTest, Parse1ToolCall1Function1ArgumentTagsNoNewline) {
136149
std::string input = R"(
137150
"<tool_call><function=string_tool><parameter=arg1>value1</parameter></function></tool_call>")";
138-
auto [generatedTensor, generatedTokens, parsedOutput] = doTheWork(input);
151+
auto [generatedTensor, generatedTokens, parsedOutput] = generateParsedOutput(input);
139152

140153
ASSERT_EQ(parsedOutput.toolCalls.size(), 1);
141154
EXPECT_EQ(parsedOutput.toolCalls[0].name, "string_tool");
@@ -152,7 +165,7 @@ value1line2
152165
</parameter>
153166
</function>
154167
</tool_call>")";
155-
auto [generatedTensor, generatedTokens, parsedOutput] = doTheWork(input);
168+
auto [generatedTensor, generatedTokens, parsedOutput] = generateParsedOutput(input);
156169

157170
ASSERT_EQ(parsedOutput.toolCalls.size(), 1);
158171
EXPECT_EQ(parsedOutput.toolCalls[0].name, "string_tool");
@@ -526,20 +539,13 @@ INSTANTIATE_TEST_SUITE_P(
526539
std::string name = std::get<0>(info.param) + "_" + std::get<2>(info.param);
527540
// Replace non-alphanumeric characters with underscore
528541
std::replace_if(name.begin(), name.end(), [](char c) { return !std::isalnum(c); }, '_');
529-
// Limit length to 30 characters
530-
if (name.length() > 30) {
531-
name = name.substr(0, 30);
532-
}
533542
return name;
534543
});
535544

536545
TEST_F(Qwen3CoderOutputParserTest, StreamingSimpleToolCall) {
537546
// since unary reuses streaming we don't need to test for partial tool calls
538547
// if we don't get closing tag we don't emit tool call
539548
int i = -1;
540-
// FIXME add content in between tool_calls and test what happens
541-
// Add another tool call to test for special tags handling
542-
// add content after second tool call
543549
std::vector<std::tuple<std::string, ov::genai::GenerationFinishReason, std::optional<std::string>>> chunkToDeltaVec{
544550
{" <too", ov::genai::GenerationFinishReason::NONE, std::nullopt},
545551
{"l_cal", ov::genai::GenerationFinishReason::NONE, std::nullopt},
@@ -564,14 +570,20 @@ TEST_F(Qwen3CoderOutputParserTest, StreamingSimpleToolCall) {
564570
{" POTENTIALLY EXISINT CONTENT", ov::genai::GenerationFinishReason::NONE, std::nullopt},
565571
{" <tool", ov::genai::GenerationFinishReason::NONE, std::nullopt},
566572
{" <tool_call>\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
567-
{"<function=string_tool", ov::genai::GenerationFinishReason::NONE, std::nullopt},
568-
{">\n", ov::genai::GenerationFinishReason::NONE, R"({"delta":{"tool_calls":[{"id":"XXXXXXXXX","type":"function","index":1,"function":{"name":"string_tool"}}]}})"},
573+
{"<function=string_int_tool", ov::genai::GenerationFinishReason::NONE, std::nullopt},
574+
{">\n", ov::genai::GenerationFinishReason::NONE, R"({"delta":{"tool_calls":[{"id":"XXXXXXXXX","type":"function","index":1,"function":{"name":"string_int_tool"}}]}})"},
569575
{"<parameter=arg1>\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
570576
{"\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
571577
{"ANOTHER_STRING_VALUE\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
572578
{"</parameter>\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
579+
{"<parameter=arg2>", ov::genai::GenerationFinishReason::NONE, std::nullopt},
580+
{"\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
581+
{"314", ov::genai::GenerationFinishReason::NONE, std::nullopt},
582+
{"1522\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
583+
{"</parameter>\n", ov::genai::GenerationFinishReason::NONE, std::nullopt},
573584
{"</function>", ov::genai::GenerationFinishReason::NONE, std::nullopt},
574-
{"</tool_call>", ov::genai::GenerationFinishReason::NONE, R"({"delta":{"tool_calls":[{"index":1,"function":{"arguments":"{\"arg1\": \"\nANOTHER_STRING_VALUE\"}"}}]}})"}};
585+
{"</tool_call>", ov::genai::GenerationFinishReason::NONE, R"({"delta":{"tool_calls":[{"index":1,"function":{"arguments":"{\"arg1\": \"\nANOTHER_STRING_VALUE\", \"arg2\": 3141522}"}}]}})"},
586+
{"CONTENT_AFTER_TOOL_CALL", ov::genai::GenerationFinishReason::NONE, std::nullopt}};
575587
for (const auto& [chunk, finishReason, expectedDelta] : chunkToDeltaVec) {
576588
i++;
577589
std::optional<rapidjson::Document> doc = outputParser->parseChunk(chunk, true, ov::genai::GenerationFinishReason::NONE);

0 commit comments

Comments
 (0)