Self-review

Author: atobiszei

src/llm/io_processing/output_parser.cpp

@@ -145,7 +145,7 @@ std::optional<rapidjson::Document> OutputParser::parseReasoningChunk(ov::genai::
 
 OutputParser::OutputParser(ov::genai::Tokenizer& tokenizer, const std::string toolParserName, const std::string reasoningParserName, const ToolsSchemas_t& toolNameSchemaMap) :
     tokenizer(tokenizer) {
-    SPDLOG_ERROR("OutputParser created with toolNameSchemaMap of size: {}", toolNameSchemaMap.size());
+    SPDLOG_TRACE("OutputParser created with toolNameSchemaMap of size: {}", toolNameSchemaMap.size());
     if (toolParserName == "llama3") {
         toolParser = std::make_unique<Llama3ToolParser>(tokenizer);
     } else if (toolParserName == "hermes3") {

src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp

@@ -55,14 +55,14 @@ static void trimNewline(std::string& str) {
     }
 }
 
-static std::string toJson(const std::vector<std::pair<std::string, std::string>>& items) {
+static std::string toJson(const ParametersValues_t& items) {
     std::ostringstream oss;
     oss << "{";
-    for (size_t i = 0; i < items.size(); ++i) {
-        const auto& [key, value] = items[i];
+    size_t i = 0;
+    for (const auto& [key, value] : items) {
         oss << "\"" << key << "\": ";
         oss << value;
-        if (i + 1 < items.size()) {
+        if (i++ + 1 < items.size()) {
             oss << ", ";
         }
     }
@@ -73,8 +73,7 @@ static std::string toJson(const std::vector<std::pair<std::string, std::string>>
 Status Qwen3CoderToolParserImpl::removeToolCallsFromContentIfNeeded(std::string& outContent) {
     if (toolsBeginStack.size() != toolsEndStack.size()) {
         SPDLOG_DEBUG("Mismatched tool tags, begin: {}, end: {}", toolsBeginStack.size(), toolsEndStack.size());
-        throw std::runtime_error("Mismatched tool tags");  // FIXME replace with status
-        return StatusCode::INTERNAL_ERROR;
+        return Status(StatusCode::INTERNAL_ERROR, "Mismatched tool tags");
     }
     while (!toolsBeginStack.empty() && !toolsEndStack.empty()) {
         auto posBegin = toolsBeginStack.top();
@@ -225,7 +224,9 @@ bool Qwen3CoderToolParserImpl::stepUntilStateChange(ToolCalls& toolCalls) {
         } else {
             parameterValue = setCorrectValueType(parameterValue, this->currentParameterName, paramIt->second);
         }
-        this->currentFunction.parameters.emplace_back(this->currentParameterName, parameterValue);
+        auto res = this->currentFunction.parameters.try_emplace(this->currentParameterName, parameterValue);
+        if (!res.second)
+            SPDLOG_DEBUG("Parameter: {} already exists", this->currentParameterName);
         this->lastProcessedPosition = pos + Qwen3CoderToolParser::parameterEndTag.length();
         this->currentState = State::InsideFunction;
         break;
@@ -387,10 +388,9 @@ std::optional<rapidjson::Document> Qwen3CoderToolParser::sendFirstDeltaIfNeeded(
 }
 
 std::optional<rapidjson::Document> Qwen3CoderToolParser::parseChunk(const std::string& newChunk, ov::genai::GenerationFinishReason finishReason) {
-    // parse Chunk needs to use streamParser to process the chunk
     // streamParser will return optional toolCalls when a tool call is completed
     // if toolCalls is returned, we need to wrap it in the required JSON structure and return it
-    // if toolCalls is not returned, but we are insideFunction state, we need to return the first delta with function name
+    // if toolCalls is not returned, but we are insideFunction state, we need to return the first delta with function name once
     // otherwise nullopt
     SPDLOG_DEBUG("Chunk: '{}', finishReason: {}", newChunk, static_cast<int>(finishReason));
     this->lazyFillInitToolParamatersTypsMap();

src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.hpp

@@ -15,7 +15,7 @@
 //*****************************************************************************
 #pragma once
 
-#include <openvino/genai/tokenizer.hpp>
+#include <map>
 #include <optional>
 #include <set>
 #include <stack>
@@ -25,6 +25,7 @@
 #include <utility>
 #include <vector>
 
+#include <openvino/genai/tokenizer.hpp>
 #pragma warning(push)
 #pragma warning(disable : 6313)
 #include <rapidjson/document.h>
@@ -37,9 +38,10 @@
 #include "src/status.hpp"
 
 namespace ovms {
+using ParametersValues_t = std::map<std::string, std::string>;
 struct Functool {
     std::string name;
-    std::vector<std::pair<std::string, std::string>> parameters;
+    ParametersValues_t parameters;
     void clear() {
         name.clear();
         parameters.clear();
@@ -106,10 +108,10 @@ class Qwen3CoderToolParser : public BaseOutputParser {
     static const std::string tagEnd;
 
 private:
-    const ToolsSchemas_t& toolSchemas;             // we need to keep reference as this is not filled in OpenAIApiHandler during ToolParser creation, NOTE that its const here but it can change outside
-    ToolsParameterTypeMap_t toolsParametersTypes;  // FIXME do it once per request
+    const ToolsSchemas_t& toolSchemas;  // we need to keep reference as this is not filled in OpenAIApiHandler during ToolParser creation, NOTE that its const here but it can change outside
+    ToolsParameterTypeMap_t toolsParametersTypes;
     bool filledParametersTypesMap{false};
-    // streaming
+    // for streaming parsing we need to keep parser as a member
     Qwen3CoderToolParserImpl streamParser;
     int toolCallIndex{-1};
     ToolCalls currentToolCalls;
@@ -125,15 +127,15 @@ class Qwen3CoderToolParser : public BaseOutputParser {
     void parse(ParsedOutput& parsedOutput, const std::vector<int64_t>& generatedTokens) override;
     std::optional<rapidjson::Document> parseChunk(const std::string& chunk, ov::genai::GenerationFinishReason finishReason) override;
     const std::string& getParsingStartTag() const override {
-        return toolsStartTag;  // FIXME CHECK
+        return toolsStartTag;
     }
     const std::unordered_set<std::string>& getSpecialParsingStartTags() const override {
-        static const std::unordered_set<std::string> specialParsingStartTags = {toolsStartTag};  // FIXME CHECK
+        static const std::unordered_set<std::string> specialParsingStartTags = {toolsStartTag};
         return specialParsingStartTags;
     }
     // Tools calls are expected to be the last part of the content, so we do not specify an end tag.
     const std::string& getParsingEndTag() const override {
-        return toolsEndTag;  // FIXME CHECK
+        return toolsEndTag;
     }
 
 private:
@@ -145,7 +147,6 @@ class Qwen3CoderToolParser : public BaseOutputParser {
 template <>
 struct fmt::formatter<ovms::Qwen3CoderToolParserImpl::State> : fmt::formatter<std::string> {
     auto format(const ovms::Qwen3CoderToolParserImpl::State& state, fmt::format_context& ctx) const {
-        // use unordered_map
         std::unordered_map<ovms::Qwen3CoderToolParserImpl::State, std::string> stateMap = {
             {ovms::Qwen3CoderToolParserImpl::State::Content, "Content"},
             {ovms::Qwen3CoderToolParserImpl::State::InsideToolCall, "InsideToolCall"},