PSV2: endpoint to register pipelines #1076
Open
Conversation
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
✅ Deploy Preview for antenna-preview canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds a project-scoped pipeline registration endpoint and tests; makes ProcessingService.endpoint_url nullable to support pull-mode services; updates schemas, tasks, migration and model fields to handle null endpoints and per-project pipeline registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ViewSet as ProjectViewSet
participant Auth as Permissions
participant Project as Project Model
participant Service as ProcessingService
participant DB as Database
rect rgba(0,128,0,0.5)
Client->>ViewSet: POST /projects/{id}/pipelines (payload)
end
ViewSet->>Auth: check_custom_permission("pipelines")
Auth-->>ViewSet: allowed / denied
alt allowed
ViewSet->>ViewSet: parse AsyncPipelineRegistrationRequest
ViewSet->>Project: fetch project by id
Project-->>ViewSet: project instance
ViewSet->>Service: get_or_create(name) / find existing
Service-->>ViewSet: service instance
ViewSet->>Service: create_pipelines(pipelines, project=project)
Service->>DB: persist pipeline configs / associations
Service-->>ViewSet: PipelineRegistrationResponse
ViewSet-->>Client: 200 + response.dict()
else denied
ViewSet-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
carlosgjs
changed the title
👷 Deploy request for antenna-ssec pending review.Visit the deploys page to approve it
|
5 tasks
carlosgjs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ami/main/api/views.py`:
- Around line 240-262: The current logic in the view returns a 400 when a
ProcessingService (found via
ProcessingService.objects.filter(name=parsed.processing_service_name).first())
is already associated with the project, which prevents idempotent
re-registration; update the branch handling the existing processing_service so
that if project is already in processing_service.projects.all() you do not
return Response(status=400) but simply continue (no-op) and allow the endpoint
to proceed (e.g., log an info/debug message) — ensure you keep the
processing_service.projects.add(project)/save() when association is missing, and
remove the early return that blocks subsequent pipeline registration.
In `@ami/ml/schemas.py`:
- Around line 326-327: The endpoint_url annotation is optional but currently
still required because it has no default; update the schema so endpoint_url has
an explicit default (e.g., set endpoint_url to None or use Field(default=None)
if this is a Pydantic model) so parsing succeeds when senders omit it—modify the
endpoint_url declaration (near the latency: float field) to include the default
None.
In `@ami/ml/tasks.py`:
- Around line 112-114: The current check only skips when service.endpoint_url is
None but still allows empty strings; update the conditional around
service.endpoint_url in the loop (the block that calls logger.warning and
continue) to treat None, empty, or whitespace-only values as missing — e.g.,
ensure you test both falsy and stripped emptiness (safe-check to avoid calling
.strip() on None) so that logger.warning(f"Processing service {service} has no
endpoint URL, skipping.") is used and the loop continues for None/"", or
whitespace-only endpoint_url.
🧹 Nitpick comments (1)
ami/main/tests.py (1)
3457-3459: Consider disabling default fixtures for these API tests.
create_defaults=Truebuilds extra related objects these tests don’t use; turning it off keeps setup lean.♻️ Suggested change
- self.project = Project.objects.create(name="Test Project", owner=self.user, create_defaults=True) - self.other_project = Project.objects.create(name="Other Project", owner=self.other_user, create_defaults=True) + self.project = Project.objects.create(name="Test Project", owner=self.user, create_defaults=False) + self.other_project = Project.objects.create(name="Other Project", owner=self.other_user, create_defaults=False)
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new API endpoint for V2 ML processing services to register pipelines with projects in "pull mode" (without requiring an endpoint URL). The changes enable processing services to push pipeline configurations to Antenna rather than Antenna pulling them from a service endpoint.
Changes:
- Added a new POST endpoint
/api/v2/projects/{id}/pipelines/for pipeline registration - Made ProcessingService.endpoint_url nullable to support pull-mode services
- Updated ProcessingService methods to handle null endpoint URLs gracefully
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ami/main/api/views.py | Added new pipelines action to ProjectViewSet for receiving pipeline registrations from V2 services |
| ami/main/models.py | Added permission check for "pipelines" action requiring ProjectManager role |
| ami/ml/models/processing_service.py | Made endpoint_url nullable and updated get_status() and get_pipeline_configs() to handle null values |
| ami/ml/schemas.py | Made endpoint_url nullable in ProcessingServiceStatusResponse and added AsyncPipelineRegistrationRequest schema |
| ami/ml/migrations/0026_make_processing_service_endpoint_url_nullable.py | Database migration to make endpoint_url nullable |
| ami/ml/tasks.py | Added check to skip processing services without endpoint URLs in periodic status check |
| ami/ml/tests.py | Added comprehensive tests for null endpoint_url handling |
| ami/main/tests.py | Added test suite for the new pipelines API endpoint |
| requirements/base.txt | Added explanatory comment about psycopg binary vs non-binary versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ami/ml/schemas.py`:
- Around line 339-345: Update the inaccurate docstring on
AsyncPipelineRegistrationRequest to reference AsyncPipelineRegistrationRequest
(not PipelineRegistrationResponse) and clearly state it represents the async
callback payload containing processing results; rename the output-oriented model
PipelineRegistrationResponse to a more generic name like
PipelineProcessingResult (or create a new PipelineProcessingResult model) and
adjust its fields (timestamp, success, error, pipelines_created,
algorithms_created) to be output/result-oriented (optional or excluded from
required input validation) so the
AsyncPipelineRegistrationRequest.pipeline_response type reflects a result object
rather than required caller-supplied output fields; update all references to
PipelineRegistrationResponse to the new name (or to the new model) including the
AsyncPipelineRegistrationRequest definition and related validation/serialization
logic.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ami/main/api/views.py`:
- Around line 232-233: Update the docstring in ami/main/api/views.py that
describes the AsyncPipelineRegistrationRequest payload: change the field name
from `pipelines_response` to the correct `pipeline_response` so it reads that
the list of PipelineConfigResponse objects is under the
`pipeline_response.pipelines` key; keep references to
AsyncPipelineRegistrationRequest and PipelineConfigResponse so readers can
locate the schema and adjust any adjacent wording to match the actual field
name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ami/main/api/views.py`:
- Around line 254-263: Replace the non-atomic filter().first() + create() flow
for ProcessingService with an atomic get_or_create using the
parsed.processing_service_name; call
ProcessingService.objects.get_or_create(name=parsed.processing_service_name,
defaults={...}) to obtain (processing_service, created) and then ensure the
project is associated (processing_service.projects.add(project)) regardless of
created, mirroring the get_or_create_default_processing_service() pattern to
avoid TOCTOU races.
♻️ Duplicate comments (1)
ami/main/api/views.py (1)
232-233: Docstring schema key is incorrect.The request schema exposes
pipelinesdirectly, notpipelines_response.pipelines. Please update the docstring to match the actual payload structure.📝 Proposed docstring fix
- list of PipelineConfigResponse objects under the `pipelines_response.pipelines` key. + list of PipelineConfigResponse objects under the `pipelines` key.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ami/main/api/views.py`:
- Around line 226-227: The pipelines view method currently defines an unused
parameter pk which triggers ARG002; update the method signature for def
pipelines(self, request, pk=None): to silence the lint warning by marking the
unused argument with a noqa (e.g., def pipelines(self, request, pk=None): #
noqa: ARG002) or remove/rename if appropriate; ensure the change references the
pipelines method so the decorator `@action`(detail=True, methods=["post"],
url_path="pipelines") remains unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
This pull request introduces a new API endpoint to support the registration of ML pipelines for a project, primarily for integration with V2 ML processing services. It also changes the
ProcessingService.endpoint_urlto be nullable.New API endpoint for pipeline registration:
pipelinesPOST action to the project viewset inami/main/api/views.py, allowing V2 ML processing services to register available pipelines for a project. The endpoint parses the payload using the AsyncPipelineRegistrationRequest.Closes #1086
Checklist
Summary by CodeRabbit
New Features
Permissions
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.