feat: added parsers lib #2542
feat: added parsers lib #2542
Conversation
WalkthroughAdds a new workspace crate lib/parsers with Cargo wiring. Introduces parsing framework for LLM tool calls: data models, configurable parser presets, JSON tool-call extraction/normalization, detection/dispatch, and adapters to async-openai aggregate/stream tool-call types. Exposes APIs via lib.rs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant P as parsers::detect_and_parse_tool_call
participant CFG as ToolCallConfig (presets)
participant JP as json_parser::try_tool_call_parse_json
participant R as Vec<ToolCallResponse>
C->>P: detect_and_parse_tool_call(message, parser_str)
P->>CFG: Select preset (default/hermes/...)
CFG-->>P: ToolCallConfig
P->>JP: try_tool_call_parse(message, &config)
JP->>JP: Extract tokens (regex), normalize payload(s)
JP->>JP: Deserialize to Parameters/Arguments or arrays
JP-->>P: Vec<ToolCallResponse>
P-->>C: Vec<ToolCallResponse>
sequenceDiagram
autonumber
participant C as Caller
participant T as tools::{aggregate,stream}
participant P as detect_and_parse_tool_call
participant A as async-openai types
C->>T: try_tool_call_parse_aggregate(message, parser)
T->>P: detect_and_parse_tool_call(...)
P-->>T: Vec<ToolCallResponse>
T->>A: Map to ChatCompletionMessageToolCall
A-->>T: Vec<ToolCall...
T-->>C: Vec<ToolCall>
C->>T: try_tool_call_parse_stream(message, parser)
T->>P: detect_and_parse_tool_call(...)
P-->>T: Vec<ToolCallResponse>
T->>A: Map to ToolCallChunk (indexed)
T-->>C: Vec<ToolCallChunk>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
lib/parsers/src/response.rs (1)
14-28: Consider deriving PartialEq/Eq for easier testing and comparisonsCalledFunction and ToolCallResponse currently don’t derive PartialEq/Eq. Adding them can simplify unit tests and deduplication logic.
Suggested derive update:
-#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] +#[derive(Clone, Debug, serde::Serialize, serde::Deserialize, PartialEq, Eq)] pub struct CalledFunction { pub name: String, pub arguments: String, } -#[derive(Clone, Debug, serde::Serialize)] +#[derive(Clone, Debug, serde::Serialize, PartialEq, Eq)] pub struct ToolCallResponse {lib/parsers/src/json_parser.rs (3)
39-54: Comment and implementation diverge regarding multiple matchesThe comment says “take the last one for now. TODO: Handle multiple tool calls” but the code already handles multiple by returning a JSON array of matches. Update the comment to reflect current behavior.
- // Get all matches and take the last one for now. TODO: Handle multiple tool calls + // Collect all matches. If there are multiple, return them as a JSON array string; otherwise return the single match.
216-229: List-handling comments incorrect: code returns all items, not the lastBoth list branches build results for every element, but the comments say “pop/take the last item.” Align comments with actual behavior.
- // We pop the last item in the list to use. + // Return all items in the list, preserving order. @@ - // Again, we take the last item for processing. + // Return all items in the list, preserving order.Also applies to: 241-248
61-99: Edge-case: improve single-token extraction to support fenced JSON blocksIf models emit code fences (
json ...), handle_single_token_tool_calls will currently skip them. Consider detecting fenced regions to improve recall.Happy to provide a small helper that recognizes
json/blocks and strips fences before validation.lib/parsers/src/tools.rs (3)
10-13: Docstring return-type mismatch (returns Vec, not a single item).The function returns a Vec of tool calls; the docstring states singular.
Apply this diff:
-/// If successful, returns a `ChatCompletionMessageToolCall`. +/// If successful, returns zero or more `ChatCompletionMessageToolCall`s.
36-39: Docstring return-type mismatch (streaming variant).Same issue here: it returns a Vec of chunks, not a single chunk.
Apply this diff:
-/// If successful, returns a `ChatCompletionMessageToolCallChunk`. +/// If successful, returns zero or more `ChatCompletionMessageToolCallChunk`s.
6-7: Comment nit: module name is outdated.“postprocessor module” is misleading; you’re re-exporting from
json_parserin this crate.Apply this diff:
-// Import json_parser from postprocessor module +// Re-export the JSON parser modulelib/parsers/src/parsers.rs (3)
112-121: Mistral preset likely wants parallel tool-calls tokens instead of single-call tokens.The placeholder “[TOOL_CALLS]” suggests a list/array wrapper. Consider using the
parallel_*fields so the JSON parser can correctly extract an array of calls.Apply this diff:
pub fn mistral() -> Self { Self { format: ToolCallParserType::Json, json: JsonParserConfig { - tool_call_start_tokens: vec!["[TOOL_CALLS]".to_string()], - tool_call_end_tokens: vec!["".to_string()], + parallel_tool_calls_start_tokens: vec!["[TOOL_CALLS]".to_string()], + parallel_tool_calls_end_tokens: vec!["".to_string()], ..Default::default() }, } }Please confirm the expected tokenization for Mistral tool-calls in your
json_parserimplementation (array wrapper vs. single-call tags) to ensure this preset actually matches the parser’s behavior.
162-180: Avoid per-call HashMap allocation; match on keys instead.This map is rebuilt on every invocation. A direct string match avoids allocations and is simpler.
Apply this diff:
-pub fn detect_and_parse_tool_call( - message: &str, - parser_str: Option<&str>, -) -> anyhow::Result<Vec<ToolCallResponse>> { - let mut parser_map: std::collections::HashMap<&str, ToolCallConfig> = - std::collections::HashMap::new(); - parser_map.insert("hermes", ToolCallConfig::hermes()); - parser_map.insert("nemotron_deci", ToolCallConfig::nemotron_deci()); - parser_map.insert("llama3_json", ToolCallConfig::llama3_json()); - parser_map.insert("mistral", ToolCallConfig::mistral()); - parser_map.insert("phi4", ToolCallConfig::phi4()); - parser_map.insert("default", ToolCallConfig::default()); // Add default key - - // Handle None or empty string by defaulting to "default" - let parser_key = match parser_str { - Some(s) if !s.is_empty() => s, - _ => "default", // None or empty string - }; - - match parser_map.get(parser_key) { - Some(config) => try_tool_call_parse(message, config), - None => anyhow::bail!("Parser for the given config is not implemented"), // Original message - } -} +pub fn detect_and_parse_tool_call( + message: &str, + parser_str: Option<&str>, +) -> anyhow::Result<Vec<ToolCallResponse>> { + let parser_key = match parser_str { + Some(s) if !s.is_empty() => s, + _ => "default", + }; + + let config = match parser_key { + "hermes" => ToolCallConfig::hermes(), + "nemotron_deci" => ToolCallConfig::nemotron_deci(), + "llama3_json" => ToolCallConfig::llama3_json(), + "mistral" => ToolCallConfig::mistral(), + "phi4" => ToolCallConfig::phi4(), + "default" => ToolCallConfig::default(), + other => { + anyhow::bail!( + "Unknown parser '{}'. Supported: hermes, nemotron_deci, llama3_json, mistral, phi4, default", + other + ) + } + }; + try_tool_call_parse(message, &config) +}
135-154: Not-implemented branches are fine; consider tracking follow-ups.The explicit bails for Harmony/Pythonic/Typescript/Xml are reasonable for now. Consider adding TODOs or a tracking issue link per variant so future work is discoverable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(2 hunks)lib/parsers/Cargo.toml(1 hunks)lib/parsers/src/json_parser.rs(1 hunks)lib/parsers/src/lib.rs(1 hunks)lib/parsers/src/parsers.rs(1 hunks)lib/parsers/src/response.rs(1 hunks)lib/parsers/src/tools.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2542/merge) by ayushag-nv.
Cargo.toml
[error] 32-32: Trailing whitespace check failed. The pre-commit hook modified lib/parsers/Cargo.toml (line 32). Command: 'pre-commit run --show-diff-on-failure --color=always --all-files'.
lib/parsers/Cargo.toml
[error] 32-32: Trailing whitespace check failed. The pre-commit hook modified lib/parsers/Cargo.toml (line 32). Command: 'pre-commit run --show-diff-on-failure --color=always --all-files'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (6)
Cargo.toml (2)
14-17: Workspace member wiring for lib/parsers looks goodAdding lib/parsers to the workspace members is correct and enables cargo to pick up the new crate.
30-37: Expose dynamo-parsers as a workspace dependency to simplify intra-workspace useDeclaring dynamo-parsers under [workspace.dependencies] is the right move so other crates can consume it with { workspace = true }.
lib/parsers/src/response.rs (1)
6-10: Enum for tool call type is clear and future-proofToolCallType with snake_case serialization and a Function variant aligns with OpenAI tool-calls and leaves room for other types later.
lib/parsers/src/lib.rs (1)
4-13: Nice, clean public surface via re-exportsThe module organization and re-exports provide a straightforward entrypoint for consumers.
lib/parsers/src/tools.rs (1)
24-31: Review comment resolved: original code is correctThe
r#typefield onChatCompletionMessageToolCallindeed uses theChatCompletionToolTypeenum (variantFunction), not a non-existentChatCompletionMessageToolCallType. No changes are necessary — the code as written will compile against the async-openai crate.lib/parsers/src/parsers.rs (1)
41-52: Empty-string end tokens can be ambiguous; confirm parser semantics.Using "" as an end token relies on parser behavior to mean “until end of message.” If the implementation treats empty as a valid match at position 0, it could yield surprising spans.
Can you confirm
try_tool_call_parse_jsontreats empty end tokens as “read to end” and doesn’t accidentally match at the start? If not, consider making this explicit in the parser (e.g., interpret empty as None) and document it here.
ayushag-nv
force-pushed
the
ayushag/add-parsers-lib
branch
from
1ab2b51 to
234079a
Compare
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
This PR migrates all tool calling and reasoning parsers to separate lib called parsers. With this migration it is now decoupled from the rest of the code. Can be easily migrated to a separate repo in future.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit