refactor: update Antenna API client for POST /tasks and wrapped /result#134
Merged
refactor: update Antenna API client for POST /tasks and wrapped /result#134
Conversation
Adapt to breaking API changes in RolnickLab/antenna#1197: - /jobs/{id}/tasks/ changed from GET with ?batch=N to POST with {"batch_size": N} in the request body - /jobs/{id}/result/ now expects {"results": [...]} wrapper instead of a bare list - Trailing slash is required on POST URLs (Django APPEND_SLASH cannot redirect POSTs) - processing_service_name query param removed from both endpoints Also adds typed Pydantic schemas for the new request/response shapes (AntennaTasksRequest, AntennaResultPostResponse) and updates the mock test server to use them as FastAPI body parameters. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR switches task polling from GET query requests to POST with a JSON body, removes the propagated Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker
participant Dataset as RESTDataset
participant API as AntennaAPI
participant Client as AntennaClient
Worker->>Dataset: request next batch (POST /api/v2/jobs/{id}/tasks {batch_size})
Dataset->>API: POST /api/v2/jobs/{id}/tasks/ (AntennaTasksRequest)
API-->>Dataset: 200 OK (tasks list)
Dataset-->>Worker: return tasks
Worker->>Worker: process tasks -> results list
Worker->>Client: post_batch_results(base_url, token, job_id, results)
Client->>API: POST /api/v2/jobs/{id}/result/ (AntennaTaskResults)
API-->>Client: 202 Accepted (AntennaResultPostResponse {results_queued})
Client-->>Worker: confirm queued (results_queued)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trapdata/antenna/client.py (1)
77-83:⚠️ Potential issue | 🟠 MajorRemove unused
processing_service_nameparameter from function signature and call site.The
processing_service_nameparameter is accepted and documented in the function signature but is never referenced in the implementation (it was removed from the API as noted in PR objectives). Remove it from the function definition attrapdata/antenna/client.py:77-83and update the corresponding call site attrapdata/antenna/result_posting.py:177.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trapdata/antenna/client.py` around lines 77 - 83, Remove the unused parameter processing_service_name from the post_batch_results function signature in trapdata/antenna/client.py by editing the post_batch_results definition to accept only (base_url: str, auth_token: str, job_id: int, results: list[AntennaTaskResult]) and update its type hints and docstring if present; then update the call site in trapdata/antenna/result_posting.py (the invocation of post_batch_results around line ~177) to stop passing processing_service_name (pass the existing four args only). Ensure imports and any tests referring to post_batch_results are updated accordingly.
🧹 Nitpick comments (2)
trapdata/antenna/schemas.py (1)
37-41: Consider adding validation forbatch_size.The
batch_sizefield accepts any integer, including zero or negative values. If the API expects a positive integer, consider adding a constraint.♻️ Optional: Add validation constraint
class AntennaTasksRequest(pydantic.BaseModel): """Request body for POST /api/v2/jobs/{job_id}/tasks/.""" - batch_size: int + batch_size: int = pydantic.Field(gt=0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trapdata/antenna/schemas.py` around lines 37 - 41, AntennaTasksRequest currently allows any integer for batch_size; enforce that it is a positive integer by updating the AntennaTasksRequest model: change the batch_size field to use a constrained type or Field with gt=0 (e.g., pydantic.conint(gt=0) or Field(..., gt=0)) or add a `@validator` on batch_size to raise a ValueError for non-positive values so requests with zero/negative batch_size are rejected; modify the AntennaTasksRequest class accordingly.trapdata/antenna/datasets.py (1)
113-113: Remove unusedprocessing_service_nameparameter fromRESTDataset.__init__.The parameter is accepted and stored at line 132 but never referenced in any class method, including
_fetch_tasks(). Per the PR objectives removing this from endpoints, the parameter should be removed from the class to avoid confusion. Also update the call site inget_rest_dataloader()(line 437).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trapdata/antenna/datasets.py` at line 113, Remove the unused processing_service_name parameter from RESTDataset.__init__ and its stored attribute: delete the parameter from the signature and any assignment in __init__, and ensure no methods (e.g., _fetch_tasks) reference it; then update the call site in get_rest_dataloader() to stop passing processing_service_name. Verify RESTDataset.__init__, _fetch_tasks, and get_rest_dataloader are consistent after removing the parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@trapdata/antenna/client.py`:
- Around line 77-83: Remove the unused parameter processing_service_name from
the post_batch_results function signature in trapdata/antenna/client.py by
editing the post_batch_results definition to accept only (base_url: str,
auth_token: str, job_id: int, results: list[AntennaTaskResult]) and update its
type hints and docstring if present; then update the call site in
trapdata/antenna/result_posting.py (the invocation of post_batch_results around
line ~177) to stop passing processing_service_name (pass the existing four args
only). Ensure imports and any tests referring to post_batch_results are updated
accordingly.
---
Nitpick comments:
In `@trapdata/antenna/datasets.py`:
- Line 113: Remove the unused processing_service_name parameter from
RESTDataset.__init__ and its stored attribute: delete the parameter from the
signature and any assignment in __init__, and ensure no methods (e.g.,
_fetch_tasks) reference it; then update the call site in get_rest_dataloader()
to stop passing processing_service_name. Verify RESTDataset.__init__,
_fetch_tasks, and get_rest_dataloader are consistent after removing the
parameter.
In `@trapdata/antenna/schemas.py`:
- Around line 37-41: AntennaTasksRequest currently allows any integer for
batch_size; enforce that it is a positive integer by updating the
AntennaTasksRequest model: change the batch_size field to use a constrained type
or Field with gt=0 (e.g., pydantic.conint(gt=0) or Field(..., gt=0)) or add a
`@validator` on batch_size to raise a ValueError for non-positive values so
requests with zero/negative batch_size are rejected; modify the
AntennaTasksRequest class accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dc7c977-e8ad-4132-9851-6fb0310f3fca
📒 Files selected for processing (4)
trapdata/antenna/client.pytrapdata/antenna/datasets.pytrapdata/antenna/schemas.pytrapdata/antenna/tests/antenna_api_server.py
…nd dataset APIs The processing_service_name parameter was threaded through post_batch_results, ResultPoster.post_async, RESTDataset, get_rest_dataloader, and _process_job but was never sent to the Antenna API in any of these code paths. Removes the dead parameter from all affected signatures and call sites. Also adds gt=0 validation to AntennaTasksRequest.batch_size. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Addressed all review comments in 588c186:
|
5 tasks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
trapdata/antenna/result_posting.py (2)
17-18:⚠️ Potential issue | 🟡 MinorStale docstring example shows removed
service_nameparameter.The usage example still references
service_namewhich was removed from thepost_asyncsignature.📝 Proposed fix
- poster = ResultPoster(max_pending=5) - poster.post_async(base_url, auth_token, job_id, results, service_name) + poster = ResultPoster(max_pending=5) + poster.post_async(base_url, auth_token, job_id, results)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trapdata/antenna/result_posting.py` around lines 17 - 18, The docstring/example still shows calling poster.post_async with a removed parameter service_name; update the example to call poster.post_async(base_url, auth_token, job_id, results) (and leave poster.get_metrics() as-is) so it matches the current post_async signature; search for poster.post_async occurrences in the module (and any examples or tests) and remove the obsolete service_name argument from those calls.
62-65:⚠️ Potential issue | 🟡 MinorStale docstring example in class docstring.
Same issue as the module docstring - the example still includes
service_name.📝 Proposed fix
Example: poster = ResultPoster(max_pending=10) - poster.post_async(base_url, auth_token, job_id, results, service_name) + poster.post_async(base_url, auth_token, job_id, results) metrics = poster.get_metrics() poster.shutdown()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trapdata/antenna/result_posting.py` around lines 62 - 65, Update the stale example in the ResultPoster class docstring (and any similar module docstring) to match the current ResultPoster API: remove the obsolete service_name argument from the post_async call and show the correct call sequence using ResultPoster(max_pending=10), poster.post_async(base_url, auth_token, job_id, results), poster.get_metrics(), and poster.shutdown(). Ensure the docstring example references the ResultPoster class and its post_async, get_metrics, and shutdown methods so it stays accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@trapdata/antenna/result_posting.py`:
- Around line 17-18: The docstring/example still shows calling poster.post_async
with a removed parameter service_name; update the example to call
poster.post_async(base_url, auth_token, job_id, results) (and leave
poster.get_metrics() as-is) so it matches the current post_async signature;
search for poster.post_async occurrences in the module (and any examples or
tests) and remove the obsolete service_name argument from those calls.
- Around line 62-65: Update the stale example in the ResultPoster class
docstring (and any similar module docstring) to match the current ResultPoster
API: remove the obsolete service_name argument from the post_async call and show
the correct call sequence using ResultPoster(max_pending=10),
poster.post_async(base_url, auth_token, job_id, results), poster.get_metrics(),
and poster.shutdown(). Ensure the docstring example references the ResultPoster
class and its post_async, get_metrics, and shutdown methods so it stays
accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec38284c-0a5c-4350-b474-56574263c24b
📒 Files selected for processing (7)
trapdata/antenna/benchmark.pytrapdata/antenna/client.pytrapdata/antenna/datasets.pytrapdata/antenna/result_posting.pytrapdata/antenna/schemas.pytrapdata/antenna/tests/test_memory_leak.pytrapdata/antenna/worker.py
💤 Files with no reviewable changes (3)
- trapdata/antenna/tests/test_memory_leak.py
- trapdata/antenna/benchmark.py
- trapdata/antenna/worker.py
…b calls The previous commit removed processing_service_name from _process_job but missed updating the test call sites, causing "got multiple values for argument 'device'" errors. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The upstream Antenna API (RolnickLab/antenna#1197) no longer accepts processing_service_name on GET /jobs. Remove the parameter from the client, worker call site, mock server, and test assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add trailing slash to POST /tasks/ route (matches client and /result/) - Set redirect_slashes=False on mock FastAPI app so slash mismatches cause 404 instead of silent redirects - Set follow_redirects=False on all TestClient instances so redirects surface as test failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove service_name from result_posting.py docstring examples (param was removed in 588c186 but examples not updated) - Remove vestigial service_name param from benchmark run_benchmark() and CLI arg (no longer passed to any API call) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
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.
Summary
Adapts the Antenna API client to the breaking changes in RolnickLab/antenna#1197. Must be deployed simultaneously with that PR.
/jobs/{id}/tasks/— GET with?batch=Nquery param → POST with{"batch_size": N}body/jobs/{id}/result/— barelist[PipelineTaskResult]→{"results": [...]}wrapperAPPEND_SLASHcannot redirect POSTs)processing_service_namequery param removed from both endpointsAlso adds typed Pydantic schemas for the new request/response shapes and updates the mock test server to use them as FastAPI body parameters.
Changes
trapdata/antenna/datasets.pyAntennaTasksRequestschema for body, trailing slashtrapdata/antenna/client.pyAntennaTaskResults, validate response withAntennaResultPostResponsetrapdata/antenna/schemas.pyAntennaTasksRequest,AntennaResultPostResponse,QueuedTaskAcknowledgmenttrapdata/antenna/tests/antenna_api_server.pyTest plan
pytest trapdata/antenna/tests/test_worker.py)Deployment
Deploy simultaneously with RolnickLab/antenna#1197. The API contract changes are breaking in both directions.
Summary by CodeRabbit