Evaluation - #405
Conversation
- Remove project_id parameter from /evaluate endpoint - Update get_provider_credential calls to not require project_id - Credentials now retrieved via API key authentication - Clean up logging configuration and imports - Fix linting errors and update type annotations
WalkthroughAdds a full evaluation subsystem: dataset CSV upload/management, evaluation run lifecycle, batch provider abstractions with OpenAI batch integration, embedding-based scoring and Langfuse integration, cron-driven polling, DB models/migration, storage utilities, tests, docs, and small auth/endpoint adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as Evaluation API
participant DB as Database
participant Storage as Object Store
participant Langfuse
participant OpenAI as OpenAI Batch
participant Cron as Cron Worker
User->>API: POST /upload_dataset (CSV)
API->>Storage: (optional) upload CSV
API->>Langfuse: create dataset & upload items
API->>DB: store EvaluationDataset
API-->>User: DatasetUploadResponse
User->>API: POST /evaluations (dataset_id, config)
API->>DB: create EvaluationRun (pending)
API->>Langfuse: fetch dataset items
API->>API: build JSONL
API->>OpenAI: submit batch job
API->>DB: update run with batch_job_id
API-->>User: EvaluationRunPublic
Cron->>API: GET /cron/evaluations
API->>DB: query pending EvaluationRuns
loop per pending run
API->>OpenAI: poll batch status
alt complete
API->>OpenAI: download results
API->>Storage: (optional) upload results
API->>Langfuse: create traces
API->>OpenAI: start embedding batch
API->>DB: update run status/metrics
else not complete
API->>DB: no change
end
end
API-->>Cron: summary
sequenceDiagram
participant Embedding as Embedding Batch
participant OpenAI
participant Scorer
participant Langfuse
participant DB
Embedding->>OpenAI: poll embedding batch status
alt complete
Embedding->>OpenAI: download embedding results
Embedding->>Scorer: parse embeddings & pairings
Scorer->>Scorer: compute cosine similarities (per-item)
Scorer->>Scorer: aggregate mean/std
Scorer->>Langfuse: update traces with scores
Scorer->>DB: update EvaluationRun with scores
else failed
Embedding->>DB: mark run completed with error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
backend/app/api/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-10-08T12:05:01.317ZApplied to files:
📚 Learning: 2025-10-08T12:05:01.317ZApplied to files:
🧬 Code graph analysis (1)backend/app/api/routes/evaluation.py (9)
⏰ 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). (1)
🔇 Additional comments (8)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/python/invoke-cron.py (1)
148-149: Validate the response body status field to avoid masking failures.The code logs "Endpoint invoked successfully" based only on the HTTP 200 status code without checking the response body. If the API returns
{"status": "error", ...}, the failure will be masked and monitoring will miss it.Apply this diff to validate the response body:
result = await self.invoke_endpoint(client) - logger.info(f"Endpoint invoked successfully: {result}") + + # Check response body status field + status = result.get("status") + if status != "success": + error_msg = result.get("message", "Unknown error") + logger.error(f"Endpoint returned failure status: {status}, message: {error_msg}, full response: {result}") + # Continue to next iteration rather than crash the cron + else: + logger.info(f"Endpoint invoked successfully: {result}")
🧹 Nitpick comments (6)
scripts/python/invoke-cron.py (6)
18-19: Add type hints to module-level constants.Per coding guidelines, type hints should be used throughout the Python code.
Apply this diff:
-ENDPOINT = "/api/v1/cron/evaluations" # Endpoint to invoke -REQUEST_TIMEOUT = 30 # Timeout for requests in seconds +ENDPOINT: str = "/api/v1/cron/evaluations" # Endpoint to invoke +REQUEST_TIMEOUT: int = 30 # Timeout for requests in secondsAs per coding guidelines.
32-51: Add return type hint to__init__method.Per coding guidelines, all methods should have type hints.
Apply this diff:
- def __init__(self): + def __init__(self) -> None:As per coding guidelines.
53-84: Add return type hint and uselogging.exceptionin exception handlers.Per coding guidelines, type hints should be used. Additionally,
logging.exceptionis preferred overlogging.errorin exception handlers as it automatically includes the traceback.Apply this diff:
- async def authenticate(self, client: httpx.AsyncClient) -> str: + async def authenticate(self, client: httpx.AsyncClient) -> str: """Authenticate and get access token.""" logger.info("Authenticating with API...") login_data = { "username": self.email, "password": self.password, } try: response = await client.post( f"{self.base_url}/api/v1/login/access-token", data=login_data, timeout=REQUEST_TIMEOUT, ) response.raise_for_status() data = response.json() self.access_token = data.get("access_token") if not self.access_token: raise ValueError("No access token in response") logger.info("Authentication successful") return self.access_token except httpx.HTTPStatusError as e: - logger.error(f"Authentication failed with status {e.response.status_code}") + logger.exception(f"Authentication failed with status {e.response.status_code}") raise except Exception as e: - logger.error(f"Authentication error: {e}") + logger.exception(f"Authentication error: {e}") raiseAs per coding guidelines.
86-129: Add return type hint and uselogging.exceptionin exception handlers.Per coding guidelines, type hints should be used. Additionally, use
logging.exceptionin exception handlers for automatic traceback inclusion.Apply this diff:
- async def invoke_endpoint(self, client: httpx.AsyncClient) -> dict: + async def invoke_endpoint(self, client: httpx.AsyncClient) -> dict: """Invoke the configured endpoint.""" if not self.access_token: await self.authenticate(client) headers = {"Authorization": f"Bearer {self.access_token}"} # Debug: Log what we're sending logger.debug(f"Request URL: {self.base_url}{self.endpoint}") logger.debug(f"Request headers: {headers}") try: response = await client.get( f"{self.base_url}{self.endpoint}", headers=headers, timeout=REQUEST_TIMEOUT, ) # Debug: Log response headers and first part of body logger.debug(f"Response status: {response.status_code}") logger.debug(f"Response headers: {dict(response.headers)}") # If unauthorized, re-authenticate and retry once if response.status_code == 401: logger.info("Token expired, re-authenticating...") await self.authenticate(client) headers = {"Authorization": f"Bearer {self.access_token}"} response = await client.get( f"{self.base_url}{self.endpoint}", headers=headers, timeout=REQUEST_TIMEOUT, ) response.raise_for_status() return response.json() except httpx.HTTPStatusError as e: - logger.error( + logger.exception( f"Endpoint invocation failed with status {e.response.status_code}: {e.response.text}" ) raise except Exception as e: - logger.error(f"Endpoint invocation error: {e}") + logger.exception(f"Endpoint invocation error: {e}") raiseAs per coding guidelines.
131-168: Add return type hint and uselogging.exceptionfor error logging.Per coding guidelines, add return type hint. Also prefer
logging.exceptionin exception handlers.Apply this diff:
- async def run(self): + async def run(self) -> None: """Main loop to invoke endpoint periodically.""" logger.info(f"Using API Base URL: {self.base_url}") logger.info( f"Starting cron job - invoking {self.endpoint} every {self.interval_minutes} minutes" ) # Use async context manager to ensure proper cleanup async with httpx.AsyncClient() as client: # Authenticate once at startup await self.authenticate(client) while True: try: start_time = datetime.now() logger.info(f"Invoking endpoint at {start_time}") result = await self.invoke_endpoint(client) logger.info(f"Endpoint invoked successfully: {result}") # Calculate next invocation time elapsed = (datetime.now() - start_time).total_seconds() sleep_time = max(0, self.interval_seconds - elapsed) if sleep_time > 0: logger.info( f"Sleeping for {sleep_time:.1f} seconds until next invocation" ) await asyncio.sleep(sleep_time) except KeyboardInterrupt: logger.info("Shutting down gracefully...") break except Exception as e: - logger.error(f"Error during invocation: {e}") + logger.exception(f"Error during invocation: {e}") # Wait before retrying on error logger.info(f"Waiting {self.interval_seconds} seconds before retry") await asyncio.sleep(self.interval_seconds)As per coding guidelines.
171-189: Add return type hint and uselogging.exceptionfor fatal errors.Per coding guidelines, add return type hint. Also use
logging.exceptionto capture traceback for fatal errors.Apply this diff:
-def main(): +def main() -> None: """Entry point for the script.""" # Load environment variables env_path = Path(__file__).parent.parent.parent / ".env" if env_path.exists(): load_dotenv(env_path) logger.info(f"Loaded environment from {env_path}") else: logger.warning(f"No .env file found at {env_path}") try: invoker = EndpointInvoker() asyncio.run(invoker.run()) except KeyboardInterrupt: logger.info("Interrupted by user") sys.exit(0) except Exception as e: - logger.error(f"Fatal error: {e}") + logger.exception(f"Fatal error: {e}") sys.exit(1)As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.example(1 hunks)scripts/python/invoke-cron.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.env.example
📄 CodeRabbit inference engine (CLAUDE.md)
Provide .env.example as the template for .env
Files:
.env.example
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
scripts/python/invoke-cron.py
🪛 Ruff (0.14.3)
scripts/python/invoke-cron.py
1-1: Shebang is present but file is not executable
(EXE001)
49-51: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Abstract raise to an inner function
(TRY301)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Consider moving this statement to an else block
(TRY300)
80-80: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
83-83: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
123-125: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
128-128: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
164-164: Do not catch blind exception: Exception
(BLE001)
165-165: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
187-187: Do not catch blind exception: Exception
(BLE001)
188-188: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
.env.example (1)
26-30: LGTM! Clear documentation for new configuration.The new environment variables are well-documented with sensible defaults. The comments clearly explain their purpose and default behavior, making it easy for developers to understand the configuration options.
| @@ -0,0 +1,193 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Make the file executable to match the shebang.
The shebang is present but the file lacks executable permissions.
Run this command to fix:
chmod +x scripts/python/invoke-cron.py🧰 Tools
🪛 Ruff (0.14.3)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In scripts/python/invoke-cron.py around line 1, the file contains a shebang but
is missing executable permissions; make the file executable by setting its
executable bit locally (so the shebang is honored), then commit the permission
change to the repo so the script runs as intended.
kartpop
left a comment
There was a problem hiding this comment.
approved, lets merge in and release to staging; remnant issues can be taken up in the coming weeks and released with the unified API
Summary
Target issue is #417
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Here is the dataflow
Upload CSV → Langfuse Dataset (5x duplication)
Start Evaluation → Create EvaluationRun (pending)
Fetch Dataset → Build JSONL for OpenAI Batch API
Upload JSONL → OpenAI Files API
Create Batch → OpenAI Batch API (returns batch_id)
Update DB → BatchJob + EvaluationRun (processing)
Wait → OpenAI processes batch async (1-24 hours)
Celery Beat → Polls every 60 seconds
Check Status → Poll OpenAI batch status
When Response Batch Completed:
Build Embedding JSONL → [output, ground_truth] pairs per trace
Create Embedding Batch → OpenAI Embeddings API (returns batch_id)
Update DB → Link embedding_batch_job_id (status: processing)
Wait → OpenAI processes embeddings (1-24 hours)
Celery Beat → Polls embedding batch status
When Embedding Batch Completed:
User views results via API or Langfuse UI
Summary by CodeRabbit
New Features
Bug Fixes