-
Notifications
You must be signed in to change notification settings - Fork 164
feat(BA-2508): Implement artifact import delegation on remote reservoir registry #6221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| from ai.backend.manager.services.artifact_revision.actions.base import ArtifactRevisionAction | ||
|
|
||
|
|
||
| # TODO: Make this a batch action. |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| ) | ||
|
|
||
| try: | ||
| # TODO: Improve this |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| self, artifact_type: Optional[ArtifactType], registry_id_or_none: Optional[uuid.UUID] | ||
| ) -> ArtifactRegistryData: | ||
| if registry_id_or_none is None: | ||
| # TODO: Handle `artifact_type` for other types |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements artifact import delegation for remote reservoir registries to enable importing artifacts from remote registries through delegation.
- Adds
delegate_import_revision_batchservice method with conditional delegation logic based on node type - Implements REST and GraphQL APIs for delegating artifact imports
- Introduces event handling for remote artifact import completion
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/manager/services/artifact_revision/service.py | Core service implementation for delegation logic and artifact registry resolution |
| src/ai/backend/manager/api/gql/artifact.py | GraphQL mutation implementation for delegate import |
| src/ai/backend/manager/api/artifact_registry.py | REST API endpoint for delegation import |
| src/ai/backend/manager/client/artifact_registry/reservoir_client.py | Client method for remote reservoir communication |
| src/ai/backend/manager/event_dispatcher/handlers/artifact.py | Event handler for remote import completion |
| src/ai/backend/manager/services/artifact_revision/actions/delegate_import_revision_batch.py | Action definition for batch delegation |
| tests/manager/services/artifact_revision/conftest.py | Test configuration updates for new dependency |
| src/ai/backend/manager/repositories/artifact/repository.py | Database method for updating remote status |
| src/ai/backend/manager/errors/artifact_registry.py | New error classes for delegation failures |
| src/ai/backend/manager/dto/request.py | Request model for delegation |
| src/ai/backend/common/events/event_types/artifact/anycast.py | Event type for remote import completion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,3 +1,5 @@ | |||
| import uuid | |||
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are redundant imports of uuid. Line 1 imports the uuid module, while line 3 imports UUID from uuid. Consider consolidating these imports or removing the redundant one.
| import uuid |
| # TODO: Improve this | ||
| task_ids = [] | ||
| result: list[ArtifactRevisionData] = [] | ||
| for revision_id in action.artifact_revision_ids: | ||
| import_result = await self.import_revision( | ||
| ImportArtifactRevisionAction(artifact_revision_id=revision_id) | ||
| ) | ||
| task_ids.append(import_result.task_id) | ||
| result.append(import_result.result) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sequential processing of revision imports could be inefficient for large batches. Consider using asyncio.gather() or similar concurrency patterns to process imports in parallel.
| self, artifact_type: Optional[ArtifactType], registry_id_or_none: Optional[uuid.UUID] | ||
| ) -> ArtifactRegistryData: | ||
| if registry_id_or_none is None: | ||
| # TODO: Handle `artifact_type` for other types |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment indicates incomplete implementation for handling different artifact types. This should be documented more clearly or tracked in a separate issue.
| # TODO: Handle `artifact_type` for other types | |
| # NOTE: Currently, only 'model' artifact type is supported here. | |
| # Support for other artifact types should be implemented in the future. | |
| # See issue #1234 for tracking this limitation. |
| from ai.backend.manager.services.artifact_revision.actions.base import ArtifactRevisionAction | ||
|
|
||
|
|
||
| # TODO: Make this a batch action. |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment suggests this isn't actually implemented as a batch action yet, which contradicts the class name 'BatchAction'. This should be clarified or the implementation should be updated.
| # TODO: Make this a batch action. |
| "Mismatch between artifact revisions and task IDs returned" | ||
| ) | ||
|
|
||
| for task_id, artifact_revision in zip(action_result.task_ids, artifact_revisions, strict=True): |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Good use of strict=True in zip() to ensure length consistency between task_ids and artifact_revisions.
Co-authored-by: octodog <[email protected]>
1cafe35 to
b43c2d5
Compare
resolves #6031 (BA-2508)
Checklist: (if applicable)
📚 Documentation preview 📚: https://sorna--6221.org.readthedocs.build/en/6221/
📚 Documentation preview 📚: https://sorna-ko--6221.org.readthedocs.build/ko/6221/