feat: Improve FastAPI agentic template #447
Conversation
🦋 Changeset detectedLatest commit: e0f224d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request involve enhancements to the prompt generation process and improvements to error handling in streaming responses. Two new constants, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 2
🧹 Outside diff range and nitpick comments (3)
templates/types/streaming/fastapi/app/api/routers/vercel_response.py (3)
59-60: Consider adding timeout handling for streaming responses.While the disconnection check is good, consider adding a timeout mechanism to prevent indefinite streaming:
if await request.is_disconnected(): break +if time.time() - start_time > MAX_STREAM_DURATION: + logger.warning("Stream timeout reached") + break if not is_stream_started: is_stream_started = TrueAlso applies to: 62-66
68-73: Consider providing more specific error messages.The current generic error message could be enhanced to provide more context about the failure type (e.g., network issues, processing errors) while still maintaining user-friendliness.
-error_message = ( - "An error occurred while processing your request. Please try again." -) +error_message = cls._get_user_friendly_error_message(str(e))You would need to implement
_get_user_friendly_error_messageto map common exceptions to user-friendly messages while maintaining security by not exposing sensitive details.
74-76: Consider adding metrics for stream completion.The finally block is a good place to add metrics for monitoring stream completion rates and durations.
finally: # Ensure event handler is marked as done even if connection breaks event_handler.is_done = True + # Add metrics for monitoring + stream_duration = time.time() - start_time + metrics.record_stream_completion(stream_duration, success=(not event_handler.has_error))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
helpers/env-variables.ts(2 hunks)templates/types/streaming/fastapi/app/api/routers/vercel_response.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
templates/types/streaming/fastapi/app/api/routers/vercel_response.py (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
🔇 Additional comments (3)
templates/types/streaming/fastapi/app/api/routers/vercel_response.py (2)
56-76: LGTM! Robust error handling implementation.
The error handling implementation is well-structured with proper logging and user-friendly error messages. The finally block ensures proper cleanup of resources.
56-76: Verify streaming performance impact.
The added error handling could potentially impact streaming performance. Let's verify the implementation:
✅ Verification successful
Error handling implementation follows best practices with minimal performance impact
The error handling implementation is well-designed and follows established patterns:
- Uses async/await properly within the streaming context
- Implements connection monitoring via
is_disconnected() - Handles exceptions without blocking the stream
- Ensures cleanup in the
finallyblock - Maintains similar structure to other streaming implementations in the codebase
The added error handling adds negligible overhead as it:
- Only catches exceptions outside the main streaming loop
- Uses lightweight logging
- Returns a simple error message without complex processing
- Properly closes resources via context manager
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other streaming implementations to compare patterns
rg -l "StreamingResponse|stream.merge" --type py
# Look for any performance-related configurations
rg "MAX_STREAM|TIMEOUT|BUFFER" --type py
# Check for any existing error handling patterns
ast-grep --pattern 'try {
$$$
stream.$$$
$$$
} catch ($$$) {
$$$
}'
Length of output: 939
Script:
#!/bin/bash
# Let's examine the streaming implementations in detail
rg -A 10 "async def stream" --type py
# Check for any async streaming patterns and error handling
rg -A 10 "async with.*stream" --type py
# Look for any connection handling patterns
rg -B 2 -A 2 "is_disconnected" --type py
Length of output: 3164
helpers/env-variables.ts (1)
16-21: Verify the impact of prompt changes on existing functionality
Let's ensure these prompt changes don't break existing functionality.
Also applies to: 472-477
✅ Verification successful
Changes to default prompts are safely implemented
The verification shows that the changes are properly integrated into the codebase:
- The
DEFAULT_SYSTEM_PROMPTis only defined once inhelpers/env-variables.tsand is used as a base prompt that gets combined with other prompts. - The
DATA_SOURCES_PROMPTis conditionally appended to the system prompt only when data sources are present. - Other components (Python and TypeScript) consume these prompts through environment variables, making them runtime configurable.
- Agent-specific implementations (form-filling, financial report) have their own specialized default prompts that override the base prompt when needed.
The changes maintain the existing prompt composition pattern where tool-specific prompts and data source prompts are appended to the base prompt when required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other files that might be affected by the prompt changes
# Look for files using the SYSTEM_PROMPT environment variable
echo "Files potentially affected by SYSTEM_PROMPT changes:"
rg -l "SYSTEM_PROMPT" --type ts --type py
# Look for direct string references to the old default prompt
echo -e "\nFiles containing the old default prompt string:"
rg "You are a helpful assistant who helps users with their questions"
# Look for potential prompt processing logic
echo -e "\nFiles with prompt processing logic:"
rg -l "systemPrompt|SYSTEM_PROMPT" --type ts --type py
Length of output: 1526
Script:
#!/bin/bash
# Let's examine how these prompts are actually used in the affected files
# Check the usage context in TypeScript files
echo "=== TypeScript Usage Context ==="
rg "SYSTEM_PROMPT|DEFAULT_SYSTEM_PROMPT" -B 2 -A 2 --type ts
# Check the usage context in Python files
echo -e "\n=== Python Usage Context ==="
rg "SYSTEM_PROMPT|DEFAULT_SYSTEM_PROMPT" -B 2 -A 2 --type py
# Check if DATA_SOURCES_PROMPT is used
echo -e "\n=== DATA_SOURCES_PROMPT Usage ==="
rg "DATA_SOURCES_PROMPT" -B 2 -A 2
Length of output: 10313
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
templates/types/streaming/nextjs/app/components/chat-section.tsx (1)
18-24: Consider enhancing error presentation and logging.While the error handling logic is solid, consider these improvements:
- Replace
alert()with a more user-friendly error presentation component- Add error logging for debugging purposes
Consider this implementation:
- let errorMessage: string; - try { - errorMessage = JSON.parse(error.message).detail; - } catch (e) { - errorMessage = error.message; - } - alert(errorMessage); + let errorMessage: string; + try { + errorMessage = JSON.parse(error.message).detail; + } catch (e) { + errorMessage = error.message; + console.error('Error parsing error message:', e); + } + // Use a toast or error component instead of alert + handler.setError(errorMessage);You might want to add a UI component to display errors:
{handler.error && ( <div role="alert" className="error-toast"> {handler.error} </div> )}templates/components/multiagent/python/app/api/routers/vercel_response.py (2)
113-117: Add type hints and docstring for consistency.The method implementation is good but should maintain consistency with other similar methods in the class.
Consider this improvement:
@classmethod - def convert_error(cls, error: str): + def convert_error(cls, error: str) -> str: + """ + Convert error message to the streaming format. + + Args: + error: The error message to convert + + Returns: + The formatted error string for streaming + """ error_str = json.dumps(error) return f"{cls.ERROR_PREFIX}{error_str}\n"
Line range hint
31-53: Consider more granular error handling.While the current error handling is functional, it could be more informative by handling specific known error cases differently.
Consider this pattern:
try: async with stream.stream() as streamer: async for output in streamer: if not is_stream_started: is_stream_started = True yield self.convert_text("") yield output except asyncio.CancelledError: logger.info("Stopping workflow") await event_handler.cancel_run() + except ConnectionError as e: + logger.error(f"Connection error: {str(e)}", exc_info=True) + yield self.convert_error( + "Connection lost. Please check your network and try again." + ) + except TimeoutError as e: + logger.error(f"Timeout error: {str(e)}", exc_info=True) + yield self.convert_error( + "Request timed out. Please try again." + ) except Exception as e: logger.error( f"Unexpected error in content_generator: {str(e)}", exc_info=True ) yield self.convert_error( "An unexpected error occurred while processing your request, preventing the creation of a final answer. Please try again." )templates/types/streaming/fastapi/app/api/routers/vercel_response.py (2)
57-76: Robust error handling implementation with proper cleanupThe enhanced error handling is well-implemented with several good practices:
- Proper connection state checking
- User-friendly error messages
- Guaranteed cleanup with finally block
- Error logging for debugging
Consider enhancing the error logging to include more context:
- logger.exception("Error in stream response") + logger.exception("Error in stream response for chat_data: %s", chat_data)
144-147: LGTM: Consistent error message formatting methodThe convert_error method follows the established pattern of other convert methods and properly handles message escaping.
Consider adding a return type hint for consistency:
- def convert_error(cls, error: str): + def convert_error(cls, error: str) -> str:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
templates/components/multiagent/python/app/api/routers/vercel_response.py(2 hunks)templates/types/streaming/fastapi/app/api/routers/vercel_response.py(3 hunks)templates/types/streaming/nextjs/app/components/chat-section.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
templates/components/multiagent/python/app/api/routers/vercel_response.py (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
templates/types/streaming/fastapi/app/api/routers/vercel_response.py (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
templates/types/streaming/nextjs/app/components/chat-section.tsx (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
🔇 Additional comments (2)
templates/types/streaming/nextjs/app/components/chat-section.tsx (1)
18-24: LGTM! Robust error handling implementation.
The error handling implementation properly handles both JSON-parsed errors and fallback to raw error messages.
templates/types/streaming/fastapi/app/api/routers/vercel_response.py (1)
25-25: LGTM: ERROR_PREFIX constant follows established pattern
The constant follows the existing prefix pattern and is appropriately placed with other prefix constants.
templates/components/multiagent/python/app/api/routers/vercel_response.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
templates/components/engines/typescript/agent/tools/interpreter.ts (1)
119-121: LGTM! Consider improving type safety and error handling.The conversion to ArrayBuffer is correct and aligns with the sandbox requirements.
Consider these improvements:
- const content = fs.readFileSync(localFilePath); - const arrayBuffer = new Uint8Array(content).buffer; + const content = fs.readFileSync(localFilePath); + // Explicitly type the buffer conversion for better type safety + const uint8Array = new Uint8Array(content); + const arrayBuffer: ArrayBuffer = uint8Array.buffer;Also, consider adding specific error handling for file read and conversion operations:
try { for (const filePath of input.sandboxFiles) { const fileName = path.basename(filePath); const localFilePath = path.join(this.uploadedFilesDir, fileName); - const content = fs.readFileSync(localFilePath); - const arrayBuffer = new Uint8Array(content).buffer; - await this.codeInterpreter?.files.write(filePath, arrayBuffer); + try { + const content = fs.readFileSync(localFilePath); + const uint8Array = new Uint8Array(content); + const arrayBuffer: ArrayBuffer = uint8Array.buffer; + await this.codeInterpreter?.files.write(filePath, arrayBuffer); + } catch (error) { + if (error instanceof Error) { + console.error(`Failed to process file ${fileName}: ${error.message}`); + } + throw error; // Re-throw to be caught by outer try-catch + } } } catch (error) { - console.error("Got error when uploading files to sandbox", error); + console.error("Failed to upload files to sandbox:", error instanceof Error ? error.message : String(error)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
templates/components/engines/typescript/agent/tools/interpreter.ts(1 hunks)templates/types/streaming/nextjs/app/api/sandbox/route.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
templates/components/engines/typescript/agent/tools/interpreter.ts (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
templates/types/streaming/nextjs/app/api/sandbox/route.ts (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
🔇 Additional comments (1)
templates/types/streaming/nextjs/app/api/sandbox/route.ts (1)
95-96: LGTM! Proper binary data handling with ArrayBuffer
The conversion to ArrayBuffer using Uint8Array is the correct approach for handling binary file content, ensuring data integrity when writing to the sandbox.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
helpers/env-variables.ts(2 hunks)
🔇 Additional comments (1)
helpers/env-variables.ts (1)
472-477: System prompt construction needs improvement
The current implementation has issues with escaping and spacing as identified in the past review.
The previous review comment about improving system prompt construction robustness is still valid. Please refer to the suggested solution using JSON.stringify to properly escape special characters and ensure consistent spacing between components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
templates/types/streaming/nextjs/README-template.md (1)
Line range hint
40-40: Maintain consistency in instructions across sections.The Docker section still contains the conditional phrase "if the
./datafolder exists - otherwise, skip this step" while it was removed from the non-Docker instructions. This creates an inconsistent user experience.Consider applying the same simplification here:
-Parse the data and generate the vector embeddings if the `./data` folder exists - otherwise, skip this step: +Generate the embeddings of the documents in the `./data` directory:🧰 Tools
🪛 Markdownlint (0.35.0)
13-13: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
.changeset/moody-mangos-punch.md(1 hunks)questions/simple.ts(1 hunks)templates/types/streaming/express/README-template.md(1 hunks)templates/types/streaming/fastapi/README-template.md(1 hunks)templates/types/streaming/nextjs/README-template.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .changeset/moody-mangos-punch.md
- templates/types/streaming/express/README-template.md
- templates/types/streaming/fastapi/README-template.md
🧰 Additional context used
📓 Path-based instructions (1)
templates/types/streaming/nextjs/README-template.md (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
🔇 Additional comments (2)
templates/types/streaming/nextjs/README-template.md (1)
11-11: LGTM! Clear and concise instruction.
The simplified instruction makes the step more straightforward and easier to follow.
questions/simple.ts (1)
134-134:
Reconsider using weather tool for RAG application
The change from Wikipedia to weather tool for the RAG application type raises some concerns:
- RAG (Retrieval Augmented Generation) typically involves document retrieval and augmentation, while weather is more suited for real-time API calls.
- This change might not align with user expectations when they select "Agentic RAG" from the menu.
Let's verify the impact of this change:
Consider the following recommendations:
- Keep the Wikipedia tool for RAG as it better aligns with RAG's purpose
- If weather functionality is needed, consider:
- Adding it as an additional tool rather than a replacement
- Creating a separate application type for weather-based use cases
- Renaming the "Agentic RAG" option to better reflect its new purpose
Summary by CodeRabbit
New Features
Bug Fixes