[WIP] added dockerfile and agentcore required apis #148
Conversation
WalkthroughAdds a Dockerfile; introduces a new FastAPI agentcore router with Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant FastAPI
participant BackgroundTasks
participant EvaluationService
Client->>FastAPI: POST /invocations (EvaluationRequest)
FastAPI->>EvaluationService: dependency injection (get_evaluation_service)
FastAPI->>FastAPI: enqueue_evaluation(request, ..., endpoint="/invocations")
FastAPI->>BackgroundTasks: add_task(evaluate_job, job_id)
FastAPI->>Client: return EvaluationResponse (job metadata)
BackgroundTasks->>EvaluationService: execute queued evaluation job
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rogue/server/api/evaluation.py (1)
47-57: Bug:endpointparameter is not used in logging.The
endpointparameter is passed to this function but line 49 hardcodes"/evaluations"instead of using the parameter. This will cause incorrect logging for requests coming through/invocations.Proposed fix
# Build extra logging info extra_info = { - "endpoint": "/evaluations", + "endpoint": endpoint, "method": "POST", "agent_url": str(request.agent_config.evaluated_agent_url), "scenario_count": scenario_count, "judge_llm": request.agent_config.judge_llm, "deep_test_mode": request.agent_config.deep_test_mode, "max_retries": request.max_retries, "timeout_seconds": request.timeout_seconds, }
🧹 Nitpick comments (5)
rogue/server/api/agentcore.py (2)
9-11: Add return type hint for completeness.Per coding guidelines, use type hints for all function signatures.
Suggested fix
@router.get("/ping") -def ping(): +def ping() -> dict[str, str]: return {"status": "Healthy"}
14-26: Add return type hint to the function signature.Per coding guidelines, all function signatures should have type hints. While
response_model=EvaluationResponsehandles serialization, the function signature should also declare its return type.Suggested fix
@router.post("/invocations", response_model=EvaluationResponse) async def invocations( request: EvaluationRequest, background_tasks: BackgroundTasks, evaluation_service: EvaluationService = Depends(get_evaluation_service), -): +) -> EvaluationResponse: # different entrypoint to /evaluations to meet agentcore convention return await enqueue_evaluation( request=request, background_tasks=background_tasks, evaluation_service=evaluation_service, endpoint="/invocations", )rogue/server/api/evaluation.py (3)
27-32: Add return type hint toenqueue_evaluation.Per coding guidelines, use type hints for all function signatures.
Suggested fix
async def enqueue_evaluation( request: EvaluationRequest, background_tasks: BackgroundTasks, evaluation_service: EvaluationService, endpoint: str, -): +) -> EvaluationResponse:
85-96: Add return type hint tocreate_evaluation.Per coding guidelines, function signatures should include type hints.
Suggested fix
@router.post("", response_model=EvaluationResponse) async def create_evaluation( request: EvaluationRequest, background_tasks: BackgroundTasks, evaluation_service: EvaluationService = Depends(get_evaluation_service), -): +) -> EvaluationResponse: return await enqueue_evaluation(
116-125: Consider using structured logging instead of f-string.Line 122 uses an f-string in the logger call. Structured logging with
extradict is more consistent with the logging patterns used elsewhere in this file and allows better log parsing.Suggested fix
job = await evaluation_service.get_job(job_id) - logger.info(f"Job: {job}") + logger.info("Retrieved evaluation job", extra={"job_id": job_id, "job": str(job)}) if not job:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfilerogue/server/api/agentcore.pyrogue/server/api/evaluation.pyrogue/server/main.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useblackfor code formatting
Use type hints for all function signatures
Follow PEP 8 naming conventions (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Useisortconventions for import organization
Files:
rogue/server/main.pyrogue/server/api/agentcore.pyrogue/server/api/evaluation.py
⏰ 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). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
🔇 Additional comments (6)
Dockerfile (2)
29-34: LGTM!The HEALTHCHECK configuration is well-structured with appropriate intervals, timeout, and start period. The alignment with the
/pingendpoint inagentcore.pyensures proper container orchestration integration.
16-21: Fix COPY instruction for optional.python-versionfile.The
COPY .python-version* ./on line 18 will fail during Docker build because no.python-versionfile exists in the repository. Either add the file if it's required, or modify the COPY instruction to handle the missing pattern gracefully (e.g., use a wildcard approach that won't fail, or remove this line if the file is truly optional).⛔ Skipped due to learnings
Learnt from: CR Repo: qualifire-dev/rogue PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-01-04T16:10:56.623Z Learning: Applies to pyproject.toml : Use `uv` for dependency management and add new dependencies to `pyproject.toml`rogue/server/main.py (2)
31-31: LGTM!The import follows the existing pattern and is correctly placed with other router imports.
71-71: LGTM!Registering the agentcore router with an empty prefix correctly exposes
/pingand/invocationsat the root level, meeting AgentCore runtime conventions.rogue/server/api/evaluation.py (2)
99-113: LGTM!The
list_evaluationsendpoint is correctly implemented with proper dependency injection.
128-136: LGTM!The
cancel_evaluationendpoint is correctly implemented with proper error handling and dependency injection.
Docstrings generation was requested by @giladivry. * #148 (comment) The following files were modified: * `rogue/server/api/agentcore.py` * `rogue/server/api/evaluation.py` * `rogue/server/main.py`
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rogue/server/api/evaluation.py (1)
27-57: Bug:endpointparameter is unused; logs will always show/evaluations.The
endpointparameter is accepted on line 31 but never used. Line 49 hardcodes"/evaluations"instead of using the parameter. When called from the/invocationsendpoint (per the AI summary), logs will incorrectly attribute requests to/evaluations.🐛 Proposed fix
# Build extra logging info extra_info = { - "endpoint": "/evaluations", + "endpoint": endpoint, "method": "POST",
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rogue/server/api/agentcore.pyrogue/server/api/evaluation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/server/api/agentcore.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useblackfor code formatting
Use type hints for all function signatures
Follow PEP 8 naming conventions (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Useisortconventions for import organization
Files:
rogue/server/api/evaluation.py
⏰ 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)
- GitHub Check: codestyle
🔇 Additional comments (2)
rogue/server/api/evaluation.py (2)
61-82: LGTM!The job creation, enqueueing, and background task scheduling logic is well-structured. Proper use of async/await and structured logging.
85-96: LGTM!Clean refactoring to a wrapper function. Proper use of FastAPI's
Dependsfor dependency injection while delegating to the sharedenqueue_evaluationfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rogue/server/api/evaluation.py (3)
27-32: Add return type annotation.Per coding guidelines, all function signatures should include type hints. The return type is missing.
Suggested fix
async def enqueue_evaluation( request: EvaluationRequest, background_tasks: BackgroundTasks, evaluation_service: EvaluationService, endpoint: str, -): +) -> EvaluationResponse:
36-39: Unnecessary type ignore comment.After the
is not Nonecheck on line 36, the type ofrequest.scenariosis narrowed toList[Scenario], solen()should not require a type ignore. If the type checker still complains, consider using an explicit assertion or cast instead of suppressing the warning.Suggested fix
if request.scenarios is not None: - scenario_count = len(request.scenarios) # type: ignore[arg-type] + scenario_count = len(request.scenarios) else: scenario_count = 0
122-122: Use structured logging for consistency.This f-string logging is inconsistent with the structured logging pattern (using
extra={}) used elsewhere in this file. Consider using structured logging for better observability and log aggregation.Suggested fix
- logger.info(f"Job: {job}") + logger.info("Retrieved evaluation job", extra={"job_id": job_id, "found": job is not None})
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rogue/server/api/evaluation.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useblackfor code formatting
Use type hints for all function signatures
Follow PEP 8 naming conventions (snake_case for variables and functions, PascalCase for classes)
Use try/except blocks for code that may raise exceptions
Useisortconventions for import organization
Files:
rogue/server/api/evaluation.py
🧬 Code graph analysis (1)
rogue/server/api/evaluation.py (4)
sdks/python/rogue_sdk/client.py (2)
request(79-83)create_evaluation(92-99)sdks/python/rogue_sdk/types.py (2)
EvaluationRequest(743-770)EvaluationResponse(793-798)rogue/server/services/evaluation_service.py (1)
EvaluationService(21-301)sdks/python/rogue_sdk/sdk.py (1)
create_evaluation(63-65)
⏰ 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). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
🔇 Additional comments (3)
rogue/server/api/evaluation.py (3)
1-25: LGTM!Imports are well-organized, and the singleton pattern using
@lru_cache(1)forget_evaluation_serviceis appropriate for maintaining a single instance of the evaluation service.
61-82: LGTM!The job creation, persistence, and background task scheduling are correctly implemented. The response properly reflects the pending status.
85-96: LGTM!Clean refactoring that delegates to the shared
enqueue_evaluationfunction. The endpoint parameter enables proper logging differentiation when called from different routes (e.g.,/invocationsin agentcore.py).
giladivry
changed the title
Description
added dockerfile and agentcore required apis
Motivation and Context
in order to list rogue on agentcore runtime, APIs should comply with naming conventions
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs