Optimize fixed output system prompts #32
Conversation
Summary of ChangesHello @aichy126, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThe PR converts the library from asynchronous to synchronous processing across core, LLM provider, and utilities, changes output instruction markers to XML-style tags, updates preserved-content handling, and bumps the package version to 0.2.19. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MarkdownFlow
participant LLMProvider
Caller->>MarkdownFlow: process(block_index, mode, ...)
Note right of MarkdownFlow: build messages (sync)\ndetect has_preserved_content
MarkdownFlow->>LLMProvider: complete(messages)
LLMProvider-->>MarkdownFlow: response (string)
MarkdownFlow-->>Caller: LLMResult (or generator of LLMResult)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
markdown_flow/constants.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🪛 Ruff (0.14.1)markdown_flow/constants.py96-96: String contains ambiguous (RUF001) 101-101: String contains ambiguous (RUF001) 109-109: String contains ambiguous (RUF001) 111-111: String contains ambiguous (RUF001) 124-124: String contains ambiguous (RUF001) 126-126: String contains ambiguous (RUF001) 149-149: String contains ambiguous (RUF001) 153-153: String contains ambiguous (RUF001) 154-154: String contains ambiguous (RUF001) 🔇 Additional comments (2)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on optimizing the fixed output system prompts within the markdown-flow library. The primary changes involve replacing the original output instruction markers with <preserve_or_translate> tags and updating the process_output_instructions function to return a tuple indicating whether preserved content exists. Additionally, the process methods in core.py have been refactored to be synchronous rather than asynchronous, and the LLM provider interface has been updated accordingly. The version number in __init__.py has also been updated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
markdown_flow/llm.py (1)
41-68: Critical: Removal of async/await violates coding guidelines.The conversion of
completeandstreammethods from async to synchronous directly contradicts the established coding guidelines, which explicitly state: "Use async/await for LLM calls and I/O to avoid blocking."LLM API calls are I/O-bound operations that benefit significantly from async/await patterns. Removing async will:
- Block the calling thread during network I/O
- Prevent efficient concurrent processing
- Degrade performance in multi-request scenarios
- Make it impossible to use connection pooling effectively
This architectural change needs careful reconsideration. If synchronous operation is required for specific use cases, consider maintaining both async and sync interfaces, or wrapping async calls appropriately.
As per coding guidelines and learnings.
🧹 Nitpick comments (1)
markdown_flow/core.py (1)
230-230: Consider removing unused method arguments.Static analysis identified unused
contextparameters at lines 230 and 349, and an unusedblock_indexparameter at line 557. If these parameters are not needed for the current implementation and are not required for interface consistency, consider removing them to improve code clarity.Also applies to: 349-349, 557-557
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
markdown_flow/__init__.py(2 hunks)markdown_flow/constants.py(2 hunks)markdown_flow/core.py(25 hunks)markdown_flow/llm.py(3 hunks)markdown_flow/utils.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: All code (comments, identifiers, logs, error messages) must be written in English
Do not hardcode API keys or secrets in code; use environment variables or config
Prefer and maintain type hints; use MyPy for type checking
Use Ruff for linting (ruff check --fix) and formatting (ruff format)
Python modules should use snake_case for filenames
Maintain Python 3.10+ compatibility
Files:
markdown_flow/constants.pymarkdown_flow/utils.pymarkdown_flow/core.pymarkdown_flow/__init__.pymarkdown_flow/llm.py
markdown_flow/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Define and use pre-compiled regex patterns in constants.py
Files:
markdown_flow/constants.py
markdown_flow/{core,enums,exceptions,llm,models,utils}.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not compile regex inline in modules; import compiled patterns from constants.py
Files:
markdown_flow/utils.pymarkdown_flow/core.pymarkdown_flow/llm.py
markdown_flow/{llm,core}.py
📄 CodeRabbit inference engine (AGENTS.md)
Use async/await for LLM calls and I/O to avoid blocking
Files:
markdown_flow/core.pymarkdown_flow/llm.py
markdown_flow/core.py
📄 CodeRabbit inference engine (AGENTS.md)
markdown_flow/core.py: Reuse a single LLM provider instance across requests to enable connection reuse
Prefer streaming responses for large documents where possible
Cache parsed blocks and variable extractions (e.g., with lru_cache) to avoid recomputation
Files:
markdown_flow/core.py
markdown_flow/{core,llm}.py
📄 CodeRabbit inference engine (AGENTS.md)
Minimize prompt tokens while maintaining required context
Files:
markdown_flow/core.pymarkdown_flow/llm.py
markdown_flow/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
Update version number in markdown_flow/init.py for releases
Files:
markdown_flow/__init__.py
markdown_flow/llm.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement retry logic with exponential backoff for LLM calls
Files:
markdown_flow/llm.py
🧠 Learnings (2)
📚 Learning: 2025-09-20T17:36:43.544Z
Learnt from: CR
PR: ai-shifu/markdown-flow-agent-py#0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T17:36:43.544Z
Learning: Applies to markdown_flow/{llm,core}.py : Use async/await for LLM calls and I/O to avoid blocking
Applied to files:
markdown_flow/core.pymarkdown_flow/llm.py
📚 Learning: 2025-09-20T17:36:43.544Z
Learnt from: CR
PR: ai-shifu/markdown-flow-agent-py#0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T17:36:43.544Z
Learning: Applies to markdown_flow/__init__.py : Update version number in markdown_flow/__init__.py for releases
Applied to files:
markdown_flow/__init__.py
🧬 Code graph analysis (1)
markdown_flow/core.py (3)
markdown_flow/llm.py (6)
ProcessMode(15-20)complete(41-53)complete(74-75)LLMResult(24-34)stream(56-68)stream(77-78)markdown_flow/enums.py (1)
BlockType(21-30)markdown_flow/utils.py (3)
InteractionType(169-177)process_output_instructions(561-655)replace_variables_in_text(744-796)
🪛 Ruff (0.14.1)
markdown_flow/constants.py
105-105: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
107-107: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
127-127: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
markdown_flow/core.py
230-230: Unused method argument: context
(ARG002)
349-349: Unused method argument: context
(ARG002)
557-557: Unused method argument: block_index
(ARG002)
🔇 Additional comments (6)
markdown_flow/utils.py (1)
561-655: LGTM! Breaking change is well-documented.The function signature change to return a tuple is a breaking change, but it's clearly documented in the docstring. The implementation correctly tracks whether output instructions were found and returns both the processed content and the flag.
markdown_flow/__init__.py (2)
35-40: Documentation correctly updated for synchronous API.The usage examples have been properly updated to reflect the synchronous API by removing
awaitkeywords.
86-86: Verify the large version jump.The version jumped from 0.2.5 to 0.2.18 (13 versions). This is a significant jump for a single PR. Please confirm this is the intended version number.
Based on coding guidelines.
markdown_flow/constants.py (2)
50-51: LGTM! Instruction markers updated to XML format.The change from Chinese markers
[输出]to XML-style<preserve_or_translate>tags improves clarity and follows a more standardized format.
93-131: LGTM! Improved instruction structure.The new
OUTPUT_INSTRUCTION_EXPLANATIONprovides a much more structured and detailed explanation using XML-style tags. The fullwidth commas flagged by Ruff are intentional as they appear in Chinese text content.markdown_flow/core.py (1)
653-672: LGTM! Conditional instruction explanation logic is well-implemented.The logic to conditionally append
OUTPUT_INSTRUCTION_EXPLANATIONonly when preserved content is detected is a good optimization. This avoids adding unnecessary instructions when they're not needed.The implementation correctly:
- Unpacks the tuple from
process_output_instructions- Checks the
has_preserved_contentflag- Handles both cases: with and without document_prompt
… that the content must be output as it is
|
1 participant
Summary by CodeRabbit
Breaking Changes
Documentation
Chores
Behavior