feat(evaluation): Added the callback support for notifications in Evaluation Run - #1115
Open
Ayush8923 wants to merge 4 commits into
Open
feat(evaluation): Added the callback support for notifications in Evaluation Run#1115Ayush8923 wants to merge 4 commits into
Ayush8923 wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughV2 evaluations now accept optional HTTPS callback URLs. The URL is persisted with the run, and terminal transitions enqueue a Celery task that sends success or failure webhook payloads with best-effort delivery. ChangesEvaluation callback workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvaluationRun
participant send_eval_completion_callback
participant execute_eval_completion_callback
participant CallbackEndpoint
EvaluationRun->>send_eval_completion_callback: terminal transition dispatch
send_eval_completion_callback->>execute_eval_completion_callback: evaluation_id
execute_eval_completion_callback->>CallbackEndpoint: signed success or failure payload
CallbackEndpoint-->>execute_eval_completion_callback: delivery result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Ayush8923
commented
Aug 6, 2026
Comment on lines
+523
to
+532
| class EvalCompletionCallbackData(SQLModel): | ||
| """Slim run snapshot for the completion webhook — identity + status only.""" | ||
|
|
||
| id: int | ||
| run_name: str | ||
| dataset_name: str | ||
| status: str | ||
| run_mode: RunModeEnum | ||
| inserted_at: datetime | ||
| updated_at: datetime |
Collaborator
Author
There was a problem hiding this comment.
these are all the parameters that we are sending in the callback response.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/crud/evaluations/core.py (1)
273-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the emitting function name in the log prefix.
Line 280 emits
[update_evaluation_run]from_enqueue_eval_completion_callback. Change the prefix to[_enqueue_eval_completion_callback].As per coding guidelines, “Prefix every log line with the function name in square brackets.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/crud/evaluations/core.py` around lines 273 - 283, Update the logger.error message in _enqueue_eval_completion_callback to use [_enqueue_eval_completion_callback] as its prefix instead of [update_evaluation_run], while preserving the existing evaluation ID, error details, and traceback logging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py`:
- Around line 21-38: Apply narrow Python 3.11+ annotations throughout the
affected functions: annotate upgrade and downgrade with -> None in
backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py:21-38;
replace bare dict results with TypedDict-style narrow types in
eval_completion.py:215-260 and job_execution.py:450-456; annotate test methods
with -> None in test_evaluation_v2.py:150-256 and
test_completion_callback_trigger.py:29-122; narrow the score mapping and add
precise return annotations for test methods and helpers in
test_notifications.py:109-118 and 383-634.
In `@backend/app/models/evaluation.py`:
- Around line 523-532: Update backend/app/models/evaluation.py:523-532 so
EvalCompletionCallbackData matches the complete EvaluationRunPublic payload
while excluding callback_url. Update
backend/app/services/notifications/eval_completion.py:215-226 to serialize and
deliver that complete public run payload, including score, score_trace_url,
cost, and error_message.
In `@backend/app/services/notifications/eval_completion.py`:
- Around line 229-260: Update execute_eval_completion_callback to accept
force_send, defer completed runs when their trace URL is unavailable by
returning a retry signal instead of delivering, and preserve forced summary-only
delivery when force_send is true. In backend/app/celery/tasks/job_execution.py
lines 448-456, handle that retry signal with self.retry and, after retry
exhaustion, invoke execute_eval_completion_callback with force_send=True.
---
Nitpick comments:
In `@backend/app/crud/evaluations/core.py`:
- Around line 273-283: Update the logger.error message in
_enqueue_eval_completion_callback to use [_enqueue_eval_completion_callback] as
its prefix instead of [update_evaluation_run], while preserving the existing
evaluation ID, error details, and traceback logging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32b77b10-7909-40c6-8ee2-ee7d9d3f1dbb
📒 Files selected for processing (12)
backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.pybackend/app/api/docs/evaluation/create_evaluation_v2.mdbackend/app/api/routes/evaluations/evaluation_v2.pybackend/app/celery/tasks/job_execution.pybackend/app/crud/evaluations/core.pybackend/app/models/evaluation.pybackend/app/services/evaluations/fast.pybackend/app/services/notifications/eval_completion.pybackend/app/tests/api/routes/test_evaluation_v2.pybackend/app/tests/celery/test_notifications.pybackend/app/tests/crud/evaluations/test_completion_callback_trigger.pydocs/wiki/modules/evaluations.md
Comment on lines
+21
to
+38
| def upgrade(): | ||
| op.add_column( | ||
| "evaluation_run", | ||
| sa.Column( | ||
| "callback_url", | ||
| sa.Text(), | ||
| nullable=True, | ||
| comment=( | ||
| "Optional HTTPS webhook (v2 runs only) POSTed the run's " | ||
| "APIResponse[EvaluationRunPublic] once the run reaches a terminal " | ||
| "state (completed/failed). NULL when no callback was requested" | ||
| ), | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def downgrade(): | ||
| op.drop_column("evaluation_run", "callback_url") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add narrow annotations to every changed function.
Several changed functions omit return annotations. Several callback result annotations use bare dict. Add -> None where appropriate. Define narrow result types, such as TypedDict models, for callback task and delivery results.
backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py#L21-L38: annotate migration functions with-> None.backend/app/services/notifications/eval_completion.py#L215-L260: replace baredictreturn annotations with narrow result types.backend/app/celery/tasks/job_execution.py#L450-L456: use a narrow callback-task return type.backend/app/tests/api/routes/test_evaluation_v2.py#L150-L256: annotate test methods with-> None.backend/app/tests/crud/evaluations/test_completion_callback_trigger.py#L29-L122: annotate test methods with-> None.backend/app/tests/celery/test_notifications.py#L109-L118: narrow thescoremapping type.backend/app/tests/celery/test_notifications.py#L383-L634: annotate test methods and helper functions with narrow return types.
As per coding guidelines, “Use Python 3.11+ and provide narrow type hints for every function parameter and return value.”
📍 Affects 6 files
backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py#L21-L38(this comment)backend/app/services/notifications/eval_completion.py#L215-L260backend/app/celery/tasks/job_execution.py#L450-L456backend/app/tests/api/routes/test_evaluation_v2.py#L150-L256backend/app/tests/crud/evaluations/test_completion_callback_trigger.py#L29-L122backend/app/tests/celery/test_notifications.py#L109-L118backend/app/tests/celery/test_notifications.py#L383-L634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py`
around lines 21 - 38, Apply narrow Python 3.11+ annotations throughout the
affected functions: annotate upgrade and downgrade with -> None in
backend/app/alembic/versions/076_add_callback_url_to_evaluation_run.py:21-38;
replace bare dict results with TypedDict-style narrow types in
eval_completion.py:215-260 and job_execution.py:450-456; annotate test methods
with -> None in test_evaluation_v2.py:150-256 and
test_completion_callback_trigger.py:29-122; narrow the score mapping and add
precise return annotations for test methods and helpers in
test_notifications.py:109-118 and 383-634.
Source: Coding guidelines
OpenAPI changes 🟢 1 non-breaking changeTip Safe to merge from an API-contract perspective. Full changelog ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🟢 | POST |
/api/v2/evaluations |
added the new optional request property callback_url |
main ↔ 7ee2e956 · generated by oasdiff
Ayush8923
requested review from
AkhileshNegi
and removed request for
AkhileshNegi
August 6, 2026 17:05
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/tests/celery/test_notifications.py`:
- Around line 373-381: Update the callback schema and the related assertions in
the notification tests around _SLIM_KEYS and the referenced cases to validate
the full public evaluation-run response envelope. Remove the slim-payload
restriction and ensure fields such as scores, traces, and costs remain available
to webhook consumers.
- Around line 383-385: Add narrow annotations to the added test methods,
including test_completed_run_delivers_slim_success_payload and the other newly
introduced tests, using the repository’s established test-class self type and an
explicit -> None return annotation for each method.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9395e14b-07d0-499a-aca3-ac3a708a61db
📒 Files selected for processing (1)
backend/app/tests/celery/test_notifications.py
Comment on lines
+373
to
+381
| _SLIM_KEYS = { | ||
| "id", | ||
| "run_name", | ||
| "dataset_name", | ||
| "status", | ||
| "run_mode", | ||
| "inserted_at", | ||
| "updated_at", | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not lock the callback to a partial run response.
The PR objective requires the webhook to include the full evaluation-run response. These assertions require a slim payload and explicitly exclude fields such as scores, traces, and costs. This will prevent webhook consumers from receiving the required result data.
Update the callback schema and these tests to validate the agreed full public run-response envelope.
Also applies to: 415-421, 448-452
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/tests/celery/test_notifications.py` around lines 373 - 381,
Update the callback schema and the related assertions in the notification tests
around _SLIM_KEYS and the referenced cases to validate the full public
evaluation-run response envelope. Remove the slim-payload restriction and ensure
fields such as scores, traces, and costs remain available to webhook consumers.
Comment on lines
+383
to
+385
| def test_completed_run_delivers_slim_success_payload( | ||
| self, patched_session: Session | ||
| ): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add narrow annotations to the new test methods.
Add a narrow type annotation for self and -> None for each test method. The repository rule applies to every Python function parameter and return value.
Proposed pattern
+from typing import Self
- def test_completed_run_delivers_slim_success_payload(
- self, patched_session: Session
- ):
+ def test_completed_run_delivers_slim_success_payload(
+ self: Self, patched_session: Session
+ ) -> None:Apply the same pattern to the remaining added test methods.
Also applies to: 423-423, 470-470, 481-481, 505-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/tests/celery/test_notifications.py` around lines 383 - 385, Add
narrow annotations to the added test methods, including
test_completed_run_delivers_slim_success_payload and the other newly introduced
tests, using the repository’s established test-class self type and an explicit
-> None return annotation for each method.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Closes #1100
Summary
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.Summary by CodeRabbit
New Features
Bug Fixes
Documentation