-
Notifications
You must be signed in to change notification settings - Fork 164
feat(BA-4131): Add ImageAlias support to Strawberry GraphQL schema
#8400
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
base: main
Are you sure you want to change the base?
Conversation
aa869ed to
c2adf29
Compare
aliases field as dynamic resolver to ImageV2
|
I will request PR review after ImageV2 review |
HyeockJinKim
left a comment
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 task itself seems pointless. Please refrain from developing additional features that could be integrated into the search functionality.
Add SearchImagesAction for batch loading images with BatchQuerier: - Add ImageListResult type for search results - Add search_images method to ImageDBSource and ImageRepository - Add SearchImagesAction with BatchQuerier support - Register search_images processor in ImageProcessors Part of BEP-1039: Image ID Based Service Logic Migration (Stage 1) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add unit tests for search_images functionality: - Add TestSearchImages class to test_image_service.py for service layer - Add TestImageRepositorySearch class for repository layer with DB tests - Test pagination, filtering, ordering, and empty results Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Add SearchImagesAction for batch loading images with BatchQuerier: - Add ImageListResult type for search results - Add search_images method to ImageDBSource and ImageRepository - Add SearchImagesAction with BatchQuerier support - Register search_images processor in ImageProcessors Part of BEP-1039: Image ID Based Service Logic Migration (Stage 1) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add image DataLoader for batch loading images by IDs: - Add ImageConditions and ImageOrders classes for query building - Add load_images_by_ids DataLoader function - Register image_loader in DataLoaders class Part of BEP-1039: Image ID Based Service Logic Migration (Stage 1) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add test for empty ids returning empty list - Add test for returning images in request order - Add test for returning None for missing ids Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0d29856 to
008c489
Compare
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 the infrastructure for adding an aliases field to the ImageV2 GraphQL type using the DataLoader pattern to prevent N+1 query problems. However, the actual GraphQL field resolver is missing, which makes the feature non-functional.
Changes:
- Added action types, repository methods, service handlers, and DataLoader infrastructure for fetching image aliases by batch
- Refactored existing image loader to use ImageID type consistently
- Added comprehensive unit tests for the new dataloader functionality
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/manager/services/image/actions/get_aliases_by_image_ids.py | Defines new action types for batch-fetching aliases (has type inconsistencies) |
| src/ai/backend/manager/repositories/image/db_source/db_source.py | Implements database query for fetching aliases by image IDs |
| src/ai/backend/manager/repositories/image/repository.py | Adds repository method with resilience decorator |
| src/ai/backend/manager/services/image/service.py | Adds service handler for the action and deprecation comment |
| src/ai/backend/manager/services/image/processors.py | Registers the new processor |
| src/ai/backend/manager/api/gql/data_loader/image/aliases_loader.py | Implements DataLoader function for batch loading aliases |
| src/ai/backend/manager/api/gql/data_loader/image/loader.py | Refactors to use ImageID consistently |
| src/ai/backend/manager/api/gql/data_loader/image/init.py | Exports new loader function |
| src/ai/backend/manager/api/gql/data_loader/data_loaders.py | Adds image_aliases_loader property |
| src/ai/backend/manager/repositories/image/options.py | Adds unrelated ordering methods |
| tests/unit/manager/api/image/test_dataloader.py | Comprehensive tests for both loaders |
| changes/8400.feature.md | Changelog entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @dataclass | ||
| class GetAliasesByImageIdsActionResult(BaseActionResult): | ||
| aliases_map: dict[UUID, list[str]] |
Copilot
AI
Feb 2, 2026
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 aliases_map return type should use ImageID instead of UUID for the dictionary keys to be consistent with other action results and with the dataloader usage. The load_aliases_by_image_ids function expects ImageID types, and other similar actions like GetImageInstalledAgentsActionResult use Mapping[ImageID, ...] for consistency.
| if not image_ids: | ||
| return [] | ||
|
|
||
| action = GetAliasesByImageIdsAction(image_ids=list(image_ids)) |
Copilot
AI
Feb 2, 2026
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.
Type mismatch: the image_ids parameter is typed as Sequence[ImageID], but line 28 passes list(image_ids) to GetAliasesByImageIdsAction which expects list[UUID]. While ImageID is a NewType of UUID and this works at runtime, it may cause type checking issues.
This would be resolved if GetAliasesByImageIdsAction.image_ids uses list[ImageID] instead of list[UUID] (see the comment on that action class).
|
|
||
| from dataclasses import dataclass | ||
| from typing import override | ||
| from uuid import UUID |
Copilot
AI
Feb 2, 2026
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.
If the type is changed to use ImageID (as suggested in the comments above), you'll need to add an import: from ai.backend.common.types import ImageID (similar to how other image action files like get_image_installed_agents.py, preload_image.py, and unload_image.py do it).
| async def load_aliases_by_image_ids( | ||
| processor: ImageProcessors, | ||
| image_ids: Sequence[ImageID], | ||
| ) -> list[list[str]]: | ||
| """Batch load image aliases by image IDs. | ||
|
|
||
| Args: | ||
| processor: ImageProcessors instance. | ||
| image_ids: List of image IDs to load aliases for. | ||
|
|
||
| Returns: | ||
| List of alias lists in the same order as image_ids. | ||
| """ | ||
| if not image_ids: | ||
| return [] | ||
|
|
||
| action = GetAliasesByImageIdsAction(image_ids=list(image_ids)) | ||
| result = await processor.get_aliases_by_image_ids.wait_for_complete(action) | ||
|
|
||
| return [result.aliases_map.get(image_id, []) for image_id in image_ids] |
Copilot
AI
Feb 2, 2026
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.
This dataloader is implemented but the corresponding aliases field resolver is missing from the ImageV2GQL type definition. The entire purpose of this PR (BA-4131) is to add an aliases field to ImageV2GQL, but that field was never added. Without the field resolver in ImageV2GQL, this dataloader will never be called and the aliases feature will not work.
You need to add a field resolver method to the ImageV2GQL class (in src/ai/backend/manager/api/gql/image/types.py) that uses the image_aliases_loader to fetch aliases.
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.
\Can't we just apply the ids filter in the search operation? I don't understand why the GetAliasesByimageIdsAction is necessary. It appears that SearchAliasesAction can be unified.
|
|
||
| @dataclass | ||
| class GetAliasesByImageIdsAction(ImageAction): | ||
| image_ids: list[UUID] |
Copilot
AI
Feb 2, 2026
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 image_ids parameter should use ImageID type instead of UUID for consistency with other image actions in the codebase. Looking at similar actions like GetImageInstalledAgentsAction, PreloadImageAction, and UnloadImageAction (in the same directory), they all use list[ImageID] instead of list[UUID].
While UUID and ImageID are technically compatible (ImageID is a NewType of UUID), using ImageID provides better type safety and consistency across the action layer.
| async def load_aliases_by_image_ids( | ||
| processor: ImageProcessors, | ||
| image_ids: Sequence[ImageID], | ||
| ) -> list[list[str]]: | ||
| """Batch load image aliases by image IDs. | ||
|
|
||
| Args: | ||
| processor: ImageProcessors instance. | ||
| image_ids: List of image IDs to load aliases for. | ||
|
|
||
| Returns: | ||
| List of alias lists in the same order as image_ids. | ||
| """ | ||
| if not image_ids: | ||
| return [] | ||
|
|
||
| action = GetAliasesByImageIdsAction(image_ids=list(image_ids)) | ||
| result = await processor.get_aliases_by_image_ids.wait_for_complete(action) | ||
|
|
||
| return [result.aliases_map.get(image_id, []) for image_id in image_ids] |
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.
\Can't we just apply the ids filter in the search operation? I don't understand why the GetAliasesByimageIdsAction is necessary. It appears that SearchAliasesAction can be unified.
src/ai/backend/manager/repositories/image/db_source/db_source.py
Outdated
Show resolved
Hide resolved
aliases field as dynamic resolver to ImageV2ImageAlias support to Strawberry GraphQL schema
Resolves #8399 (BA-4131)
Summary
aliasesfield as a dynamic resolver toImageV2GQLusing the action-processor patternChanges
GetAliasesByImageIdsActionandGetAliasesByImageIdsActionResultaction typesquery_aliases_by_image_ids()method toImageDBSourcefor batch querying aliasesget_aliases_by_image_ids()method toImageRepositoryget_aliases_by_image_ids()service handler toImageServiceget_aliases_by_image_idsprocessor toImageProcessorsload_aliases_by_image_idsdataloader functionimage_aliases_loadertoDataLoadersclassaliasesfield resolver toImageV2GQLtypeTest plan
imagesV2and verifyaliasesfield returns correct alias list for each imageimageV2(id: ...)and verifyaliasesfield resolves correctlyRelated
🤖 Generated with Claude Code
📚 Documentation preview 📚: https://sorna--8400.org.readthedocs.build/en/8400/
📚 Documentation preview 📚: https://sorna-ko--8400.org.readthedocs.build/ko/8400/