feat: add simple query endpoint #458
Conversation
🦋 Changeset detectedLatest commit: 63b0d6a 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 |
WalkthroughA new query endpoint has been introduced to the FastAPI application, allowing users to perform queries related to the "llama" entity. This is achieved through the addition of a new router, Changes
Possibly related PRs
Suggested reviewers
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)
.changeset/five-goats-yawn.md (1)
4-5: Enhance the changeset descriptionConsider adding more details about the query endpoint's purpose, parameters, and response format to help users understand its functionality better.
Example:
-Add query endpoint +Add query endpoint that allows users to perform natural language queries against the indexed content. The endpoint accepts a query string parameter and returns a text response.templates/types/streaming/fastapi/app/api/routers/query.py (2)
10-10: Enhance logging implementationThe current logging setup could be more informative for production use.
Consider adding structured logging:
-logger = logging.getLogger("uvicorn") +logger = logging.getLogger("query_router") +logger.setLevel(logging.INFO)
1-25: Add comprehensive documentationThe endpoint would benefit from detailed API documentation.
Add OpenAPI documentation using FastAPI's built-in features:
@r.get("/") +@r.description(""" + Query endpoint for natural language queries against indexed content. + + Parameters: + query: Natural language query string (max 1000 characters) + + Returns: + String response containing the answer to the query + + Rate limit: 100 calls per hour + Authentication: Requires API key in X-API-Key header +""") async def query_request(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.changeset/five-goats-yawn.md(1 hunks)templates/types/streaming/fastapi/app/api/routers/__init__.py(1 hunks)templates/types/streaming/fastapi/app/api/routers/query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
templates/types/streaming/fastapi/app/api/routers/__init__.py (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
templates/types/streaming/fastapi/app/api/routers/query.py (1)
Pattern templates/**: For files under the templates folder, do not report 'Missing Dependencies Detected' errors.
🔇 Additional comments (2)
templates/types/streaming/fastapi/app/api/routers/__init__.py (1)
6-6: LGTM! Clean router integration
The query router integration follows the established pattern and maintains consistency with other router setups.
Also applies to: 12-12
templates/types/streaming/fastapi/app/api/routers/query.py (1)
14-14: Review empty config initialization
The empty dictionary passed to IndexConfig might lead to using default values that may not be appropriate for production.
Consider extracting configuration to a settings file or environment variables.
✅ Verification successful
Empty IndexConfig is appropriate in this case
The empty dictionary initialization of IndexConfig is actually correct because:
- The IndexConfig class only has one optional field
callback_managerwith a default value of None - The get_index function is designed to work with either None or an empty config, creating default configurations as needed
- The empty initialization
IndexConfig(**{})is equivalent to the default initialization used inget_indexwhen config is None
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for IndexConfig usage patterns in the codebase
ast-grep --pattern 'IndexConfig(**{$_})'
Length of output: 42
Script:
#!/bin/bash
# Let's first check for any IndexConfig usage in the codebase
rg "IndexConfig" -A 2
# Also check the specific file content to understand the context
cat "templates/types/streaming/fastapi/app/api/routers/query.py"
# Look for any configuration files that might contain index settings
fd "config|settings" --type f
Length of output: 9815
Script:
#!/bin/bash
# Let's check the IndexConfig class definition and its default values
cat "templates/types/streaming/fastapi/app/engine/index.py"
# Also check if there are any relevant settings in the config file
cat "templates/types/streaming/fastapi/app/config.py"
Length of output: 1133
| def get_query_engine() -> BaseQueryEngine: | ||
| index_config = IndexConfig(**{}) | ||
| index = get_index(index_config) | ||
| return index.as_query_engine() | ||
|
|
There was a problem hiding this comment.
Add error handling for query engine initialization
The get_query_engine function should handle potential initialization failures gracefully.
Consider adding error handling:
def get_query_engine() -> BaseQueryEngine:
+ try:
index_config = IndexConfig(**{})
index = get_index(index_config)
return index.as_query_engine()
+ except Exception as e:
+ logger.error(f"Failed to initialize query engine: {e}")
+ raise HTTPException(
+ status_code=500,
+ detail="Failed to initialize query engine"
+ )Committable suggestion skipped: line range outside the PR's diff.
| @r.get("/") | ||
| async def query_request( | ||
| query: str, | ||
| ) -> str: | ||
| query_engine = get_query_engine() | ||
| response = await query_engine.aquery(query) | ||
| return response.response |
There was a problem hiding this comment.
Enhance endpoint implementation with validation and rate limiting
The query endpoint needs additional safeguards for production use.
Consider these improvements:
+from fastapi import HTTPException, Depends
+from fastapi.security import APIKeyHeader
+from typing import Optional
+from app.core.rate_limit import rate_limiter
+
+# Add query validation
+from pydantic import BaseModel, Field
+
+class QueryRequest(BaseModel):
+ query: str = Field(..., min_length=1, max_length=1000)
+
@r.get("/")
+@rate_limiter(calls=100, period=3600) # 100 calls per hour
async def query_request(
- query: str,
+ query_params: QueryRequest,
+ api_key: Optional[str] = Depends(APIKeyHeader(name="X-API-Key", auto_error=False))
) -> str:
+ if not api_key:
+ raise HTTPException(status_code=401, detail="API key required")
+
query_engine = get_query_engine()
- response = await query_engine.aquery(query)
+ try:
+ response = await query_engine.aquery(query_params.query)
+ if len(response.response) > 10000: # Limit response size
+ return response.response[:10000] + "... (truncated)"
+ except Exception as e:
+ logger.error(f"Query failed: {e}")
+ raise HTTPException(status_code=500, detail="Query processing failed")
return response.responseCommittable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes