Make new projects able to run ML jobs, and stop an unreachable service from failing project creation - #1413
Open
mihow wants to merge 1 commit into
Open
Make new projects able to run ML jobs, and stop an unreachable service from failing project creation#1413mihow wants to merge 1 commit into
mihow wants to merge 1 commit into
Conversation
…heir images A new project was only connected to the push-mode service named by DEFAULT_PROCESSING_SERVICE_ENDPOINT. Asynchronous jobs find a worker through the project-to-service relation, and asynchronous is the dispatch mode new projects use, so a project created on a deployment served by polling workers had no way to process anything. Registering pipelines also fetches the service over HTTP from inside project creation, so a service that had moved or lost its certificate turned every new project into a server error, after the project row had already been written. - Connect a new project to every pull-mode processing service, with a config row per pipeline. No network call: those workers register their own pipelines. - Treat pipeline registration from the push-mode default as best-effort. The project is created without those pipelines and the failure is logged. - Extract configure_pipeline_for_project, shared by create_pipelines and the new path. This also fixes create_pipelines reporting the wrong pipelines as newly created, because the inner loop overwrote the flag the outer loop set. - Comment the endpoint out of the production env example, since a deployment on polling workers should not set it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnUB2nqxi92uGqisKdJKkX
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughNew project setup now attaches pull-mode processing services, configures their pipelines, and conditionally creates the default push-mode service. Service registration failures no longer prevent project creation. ChangesProcessing service setup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectManager
participant ProcessingService
participant ProjectPipelineConfig
ProjectManager->>ProcessingService: attach pull-mode services
ProcessingService->>ProjectPipelineConfig: create or reuse pipeline configurations
ProjectManager->>ProcessingService: configure default push-mode service
ProcessingService-->>ProjectManager: complete project setup
Merge Risk: ⚪ Minimal · up to New projects now attach registered pull-mode workers and continue creation when an optional push-mode service cannot register pipelines. The covered setup and failure paths indicate no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The critical transaction-handling issue and additional moderate findings remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates project initialization so new projects can use pull-mode ML workers, tolerate unreachable push-mode services, and clarifies endpoint configuration.
Changes:
- Attach new projects to pull-mode processing services.
- Make default push-service pipeline registration best-effort.
- Refactor pipeline configuration and add coverage.
- Document optional production endpoint configuration.
File summaries
| File | Review summary |
|---|---|
ami/ml/models/processing_service.py |
Critical (3 votes): broad exception handling may hide database failures inside the outer transaction. Moderate (2 votes): health checks may delay creation; (2 votes): algorithm keys use the wrong created-list; (1 vote): shared pipelines cause redundant lookups. |
ami/main/tests.py |
Moderate (1 vote): patch the status check to avoid a real retrying HTTP call during tests. |
ami/main/models.py |
Integrates processing-service setup into project defaults. |
.envs/.production/.django-example |
Documents optional push-mode endpoint configuration. |
Review details
Suppressed comments (2)
ami/main/tests.py:213
- This test still lets
ProcessingServiceManager.create()callget_status()againstunreachable.invalidbefore the patchedcreate_pipelines()runs. That performs the real retrying HTTP path and can make the suite slow or network-dependent; patchget_statushere as the preceding unrelated-service test does.
with mock.patch.object(
ProcessingService,
"create_pipelines",
side_effect=requests.exceptions.SSLError("certificate verify failed"),
):
ami/ml/models/processing_service.py:103
ProjectPipelineConfigis keyed only by(project, pipeline), but this loop configures every service's copy of a shared pipeline. When several pull workers expose the same pipelines, project creation repeats theget_or_createquery for each service (N×M lookups for N workers and M common pipelines); collect the union of pipelines and configure each project/pipeline once while still adding every service relation.
for pipeline in self.pipelines.all():
configure_pipeline_for_project(project, pipeline, enable_only)
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+382
to
+387
| try: | ||
| service.create_pipelines( | ||
| enable_only=settings.DEFAULT_PIPELINES_ENABLED, | ||
| projects=Project.objects.filter(pk=project.pk), | ||
| ) | ||
| except Exception: |
| project_pipeline_config.save() | ||
| else: | ||
| logger.debug(f"Using existing project pipeline config for {project.name} and {pipeline.name}.") | ||
| configure_pipeline_for_project(project, pipeline, enable_only) |
Comment on lines
+378
to
+381
| # Registering pipelines fetches the service's /info endpoint. Creating a project is | ||
| # otherwise a local operation, so an unreachable or misconfigured service must not | ||
| # take it down with it: the project is created without those pipelines and the | ||
| # service can be registered again later from the admin. |
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
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
Creating a project sets up everything a new project needs: a station, a capture set, and a
connection to the machine learning services that will process its images. That last part
stopped matching how the platform actually runs work. New projects are connected only to a
push-mode processing service, the kind that Antenna calls at a fixed address, named by the
DEFAULT_PROCESSING_SERVICE_ENDPOINTenvironment variable. Most work today is done bypull-mode workers, which poll Antenna for tasks instead of exposing an address, and a new
project was never connected to any of them.
That matters because an asynchronous job looks up its workers through the project-to-service
relation, and asynchronous is the dispatch mode new projects use by default. A project
created on a deployment served by polling workers therefore had no way to process anything:
it had a processing service on paper, and no worker that would ever pick up its jobs.
The same code path had a second problem. Registering a service's pipelines fetches that
service over HTTP, from inside project creation. On one of our deployments the configured
address pointed at a host that had been retired, so the certificate no longer matched, and
the request raised. Creating a project returned a server error even though the project row
had already been written, which is confusing to hit and worse to diagnose.
After this change, a new project is connected to the pull-mode workers that already serve
the platform's other projects, and a push-mode service that cannot be reached is logged
rather than allowed to fail the whole operation.
List of Changes
attach_async_processing_services()connects the project to every pull-mode service and gives it a configuration row per pipelineget_or_create_default_processing_service()is now best-effort and logs the failureconfigure_pipeline_for_project(), shared bycreate_pipelines()and the new pathcreatedflag that the per-pipeline outer loop had set, sopipelines_createdand its log line named the wrong pipelines.Only the configured default push-mode service is attached. A push-mode service someone else
registered is left alone, because it is called at its own address and should not receive an
unrelated project's work.
Detailed Description
ProcessingService.add_project()connects one project to a service using the pipelines thatservice already has, without asking it for anything.
configure_pipeline_for_project()is thesingle place that decides whether a pipeline starts enabled, honouring
DEFAULT_PIPELINES_ENABLEDin both the existing registration path and the new one.
The broad
exceptaround pipeline registration is deliberate. The call can fail with any ofseveral network and parsing errors, and the point is that none of them should take project
creation down. The failure is logged with a traceback.
Verification
Measured, not inferred:
TestProjectSetupandami.mlsuites pass, 90 tests.is connected to it with the expected enabled and disabled pipelines; a push-mode service
that is not the configured default is not attached; and project creation completes with its
other defaults intact when pipeline registration raises.
makemigrations --check --dry-runreports no changes.Still worth checking after merge: deployments that currently set
DEFAULT_PROCESSING_SERVICE_ENDPOINTshould confirm the address still answers, or unset it.Existing projects are untouched by this change, so any that were created without a pull-mode
service attached still need to be connected by hand.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation