feat: python as a subprocess reasoning parser structure and implementation #2750
Conversation
nachiketb-nvidia
requested review from
nnshah1,
tanmayv25,
piotrm-nvidia,
ptarasiewiczNV,
ryanolson,
grahamking,
paulhendricks,
biswapanda,
tmonty12,
GuanLuo,
rmccorm4,
tedzhouhk,
alec-flowers,
kkranen,
ishandhanani and
PeaBrane
as code owners
WalkthroughAdds a Python reasoning parser protocol and example, introduces a Rust PythonProcessParser to invoke Python parsers via subprocess (non-streaming and streaming), updates parser selection to accept a file path, adds minijinja for template rendering, and clarifies CLI help that a parser can be specified by path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Dyn as Dynamo
participant Sel as ReasoningParserType
participant PyProc as PythonProcessParser
Note over CLI,Dyn: --dyn-reasoning-parser=<name or path>
CLI->>Dyn: Start with parser option
Dyn->>Sel: get_reasoning_parser_from_name(name_or_path)
alt name_or_path is existing path
Sel->>PyProc: new(path)
Sel-->>Dyn: ReasoningParserWrapper(PyProc)
else known built-in name
Sel-->>Dyn: ReasoningParserWrapper(Built-in)
else unknown
Sel-->>Dyn: ReasoningParserWrapper(Basic)
end
sequenceDiagram
autonumber
participant Dyn as Dynamo
participant PyProc as PythonProcessParser
participant Py as Python runtime
participant Mod as Python parser module
Note over Dyn,PyProc: Non-streaming call
Dyn->>PyProc: detect_and_parse_reasoning(text, token_ids)
PyProc->>Py: spawn python3 -c "<rendered script>"
Py->>Mod: load <path>, find BaseReasoningParser subclass
Py->>Mod: instance.detect_and_parse_reasoning(text, token_ids)
Mod-->>Py: (normal_text, reasoning_text)
Py-->>PyProc: print two lines
PyProc-->>Dyn: ParserResult(normal_text, reasoning_text)
sequenceDiagram
autonumber
participant Dyn as Dynamo
participant PyProc as PythonProcessParser (stream)
participant Py as Python runtime
participant Mod as Python parser module
Note over Dyn,PyProc: Streaming incremental
Dyn->>PyProc: parse_reasoning_streaming_incremental(chunk, token_ids)
opt first call
PyProc->>Py: spawn python3 -u -c "<stream script>"
Py->>Mod: load module, instantiate parser
end
PyProc->>Py: write "<chunk>],[<ids>\n"
Py->>Mod: parser.parse_reasoning_streaming_incremental(...)
Mod-->>Py: (normal_text, reasoning_text)
Py-->>PyProc: two lines on stdout
PyProc-->>Dyn: ParserResult(normal_text, reasoning_text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 12
🧹 Nitpick comments (7)
lib/parsers/Cargo.toml (1)
38-38: Add a brief note on template input escaping and consider workspace-level dependency.
- Since minijinja will render Python scripts with user-provided paths/identifiers, ensure all interpolated values are safely string-escaped in the templates to avoid code injection.
- Optional: move
minijinjato[workspace.dependencies]for consistency across crates.lib/bindings/python/src/dynamo/reasoning_parser/__init__.py (2)
27-31: Tighten return-value docs and fix minor grammar.Clarify ordering and remove the double space in “parsed normal”.
Apply:- Returns: - Tuple[str, str]: A tuple containing the parsed normal text and reasoning if detected. - Either or both strings can be empty if no reasoning or no normal text is found. - (normal_text, reasoning_text) + Returns: + Tuple[str, str]: (normal_text, reasoning_text). + Either may be empty if no reasoning or no normal text is found.
43-47: Specify “delta” semantics for streaming to match Rust-side trait.Avoid ambiguity: streaming should return only newly parsed increments for this chunk.
- Returns: - Tuple[str, str]: A tuple containing the parsed normal text and reasoning if detected. - Either or both strings can be empty if no reasoning or no normal text is found. - (normal_text, reasoning_text) + Returns: + Tuple[str, str]: (normal_text_delta, reasoning_text_delta). + Implementations should return only the newly parsed delta for this chunk.components/backends/vllm/src/dynamo/vllm/args.py (1)
120-120: Clarify path requirements and default behavior in help text.Make it explicit that paths should be .py files and that the system defaults to 'basic' when unknown.
- help="Reasoning parser name for the model. Available options: 'basic', 'deepseek_r1', 'gpt_oss'. This can also be a file path to a custom Python reasoning parser implementation of the `dynamo.reasoning_parser.BaseReasoningParser` interface.", + help=("Reasoning parser name for the model. Available options: 'basic', 'deepseek_r1', 'gpt_oss'. " + "Alternatively, provide an absolute path to a .py file implementing " + "dynamo.reasoning_parser.BaseReasoningParser. Defaults to 'basic' if unknown."),lib/parsers/src/reasoning/mod.rs (1)
9-14: Re-export looks good; add tests for path vs name dispatch.The public re-export is fine. Consider adding unit tests that cover:
- A valid
.pypath dispatches to PythonProcessParser.- A non-existent path and unknown name fall back to Basic.
I can draft minimal tests under
lib/parsers/src/reasoning/tests/if helpful.lib/parsers/src/reasoning/python_process_parser.rs (2)
138-144: Consider adding stderr capture for better debugging.Currently, stderr output from the Python subprocess is lost, making it difficult to debug issues in production.
Add stderr capture for logging:
let mut child = Command::new("python3") .arg("-u") // unbuffered output so we can read stdout line-by-line .arg("-c") .arg(&script) .stdin(Stdio::piped()) .stdout(Stdio::piped()) + .stderr(Stdio::piped()) .spawn() .unwrap(); + + // Optionally spawn a thread to read stderr for debugging + let stderr = child.stderr.take(); + if let Some(stderr) = stderr { + thread::spawn(move || { + let reader = BufReader::new(stderr); + for line in reader.lines() { + if let Ok(line) = line { + eprintln!("Python stderr: {}", line); + } + } + }); + }
268-275: Test uses a relative path that may break in different environments.The hardcoded relative path
"../../lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py"assumes the test is always run from a specific directory, which may not be true in CI/CD environments.Use environment variables or construct the path dynamically:
#[test] fn test_python_process_parser() { let manifest_dir = env!("CARGO_MANIFEST_DIR"); let parser_path = std::path::Path::new(manifest_dir) .join("../../lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py"); let mut parser = PythonProcessParser::new(&parser_path.to_string_lossy()); // ... rest of the test }
📜 Review details
Configuration used: Path: .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)
components/backends/vllm/src/dynamo/vllm/args.py(1 hunks)lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py(1 hunks)lib/bindings/python/src/dynamo/reasoning_parser/__init__.py(1 hunks)lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py(1 hunks)lib/parsers/Cargo.toml(1 hunks)lib/parsers/src/reasoning/mod.rs(2 hunks)lib/parsers/src/reasoning/python_process_parser.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/bindings/python/src/dynamo/reasoning_parser/__init__.py (4)
lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py (2)
detect_and_parse_reasoning(24-48)parse_reasoning_streaming_incremental(50-103)lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py (2)
detect_and_parse_reasoning(24-48)parse_reasoning_streaming_incremental(50-103)lib/parsers/src/reasoning/mod.rs (4)
detect_and_parse_reasoning(46-46)detect_and_parse_reasoning(72-74)parse_reasoning_streaming_incremental(51-55)parse_reasoning_streaming_incremental(76-83)lib/parsers/src/reasoning/python_process_parser.rs (4)
detect_and_parse_reasoning(195-226)token_ids(198-201)token_ids(233-239)parse_reasoning_streaming_incremental(228-259)
lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py (2)
lib/bindings/python/src/dynamo/reasoning_parser/__init__.py (3)
BaseReasoningParser(7-48)detect_and_parse_reasoning(18-32)parse_reasoning_streaming_incremental(34-48)lib/parsers/src/reasoning/python_process_parser.rs (2)
detect_and_parse_reasoning(195-226)parse_reasoning_streaming_incremental(228-259)
lib/parsers/src/reasoning/mod.rs (2)
lib/parsers/src/reasoning/python_process_parser.rs (1)
new(135-165)lib/llm/src/protocols/openai/chat_completions/delta.rs (1)
new(80-125)
lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py (4)
lib/bindings/python/rust/llm/local_model.rs (1)
reasoning_parser(76-78)lib/bindings/python/src/dynamo/reasoning_parser/__init__.py (3)
BaseReasoningParser(7-48)detect_and_parse_reasoning(18-32)parse_reasoning_streaming_incremental(34-48)lib/parsers/src/reasoning/mod.rs (4)
detect_and_parse_reasoning(46-46)detect_and_parse_reasoning(72-74)parse_reasoning_streaming_incremental(51-55)parse_reasoning_streaming_incremental(76-83)lib/parsers/src/reasoning/python_process_parser.rs (2)
detect_and_parse_reasoning(195-226)parse_reasoning_streaming_incremental(228-259)
lib/parsers/src/reasoning/python_process_parser.rs (2)
lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py (2)
detect_and_parse_reasoning(24-48)parse_reasoning_streaming_incremental(50-103)lib/parsers/src/reasoning/mod.rs (4)
detect_and_parse_reasoning(46-46)detect_and_parse_reasoning(72-74)parse_reasoning_streaming_incremental(51-55)parse_reasoning_streaming_incremental(76-83)
⏰ 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: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: Build and Test - dynamo
lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py
Outdated
Show resolved
Hide resolved
lib/bindings/python/examples/basic_reasoning_parser/basic_parser.py
Outdated
Show resolved
Hide resolved
lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py
Outdated
Show resolved
Hide resolved
lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py
Outdated
Show resolved
Hide resolved
Signed-off-by: nachiketb <[email protected]>
Signed-off-by: nachiketb <[email protected]>
Signed-off-by: nachiketb <[email protected]>
Signed-off-by: nachiketb <[email protected]>
nachiketb-nvidia
force-pushed
the
nachiketb/add-python-reasoning-structue
branch
from
6ee7d78 to
913dca6
Compare
| while let Ok(line) = rx.recv() { | ||
| stdin | ||
| .write_all(line.as_bytes()) | ||
| .expect("Failed to write to stdin"); |
There was a problem hiding this comment.
You can't use expect because it stops the process. Log a tracing::error!(..) and return instead.
| } | ||
|
|
||
| // define a jinja template in static string | ||
| pub const REASONING_PYTHON_TEMPLATE: &str = r#" |
There was a problem hiding this comment.
Could you move this into a separate file and use include_str! macro? Then the file itself will get nice IDE help because it's a real .py.
| .iter() | ||
| .map(|id| id.to_string()) | ||
| .collect::<Vec<String>>() | ||
| .join(","); |
There was a problem hiding this comment.
Could you do a single iteration? u32 has a .to_string().
I'm confused by that part. How does the template / render part fit in? Why not execute the Python script directly. |
|
+1 for doing it in a sub-process. It makes packaging/ops/delivery a whole lot easier. It also makes testing their custom parser easier for users. |
@grahamking because for it seemed easier to just define an interface and call it how we want, extract output however we want. Also seemed like an easier way to send input tokens and text (and path of the implementation) to a python script. Also, for the streaming case, I thought the easiest way would be to use a very rigid structure to keep the python process alive while piping input text and tokens to that process |
Overview:
This is a proposal MR to allow users to set their reasoning parsers implemented in python, via an interface we define.
Python is called via a subprocess through a script rendered via jinja.
Summary by CodeRabbit