refactor(BA-501): Add draft vFolder handler Interface of manager #3493

seedspirit · 2025-01-17T12:03:59Z

This PR adds a skeleton interface for vFolder CRUD operations in manager, laying groundwork for reduced code duplication and improved maintainability in VFolder API handlers.

Codes added under src/ai/backend/manager/api/vfolders and test code in tests/manager/api/vfolders

Checklist: (if applicable)

Milestone metadata specifying the target backport version
Mention to the original issue
Update of end-to-end CLI integration tests in ai.backend.test
API server-client counterparts (e.g., manager API -> client SDK)
Test case(s) to:
- Demonstrate the difference of before/after
- Demonstrate the flow of abstract/conceptual models with a concrete implementation
Documentation
- Contents in the docs directory
- docstrings in public interfaces and type annotations

📚 Documentation preview 📚: https://sorna--3493.org.readthedocs.build/en/3493/

📚 Documentation preview 📚: https://sorna-ko--3493.org.readthedocs.build/ko/3493/

HyeockJinKim · 2025-01-17T12:09:15Z

src/ai/backend/manager/api/utils.py

 class BaseResponseModel(BaseModel):
-    status: Annotated[int, Field(strict=True, exclude=True, ge=100, lt=600)] = 200


Why was this changed here?

In the values returned by the VFolder API (ex. VFolder Create), there was an issue where the field called 'status' was duplicated in the returned values, causing conflicts in pants check etc. To prevent this, we changed the part that means status code but is expressed as status

class VFolderCreateResponseModel(BaseResponseModel): id: str ... cloneable: bool status: VFolderOperationStatus = Field(default=VFolderOperationStatus.READY)

https://github.com/lablup/backend.ai/pull/3493/files#diff-e35d6520e5e553d13a0a6bbd4e1ff47002cfd11ebd5ecc5fcd380495ee1cfac0R135

If you want to specify the status_code, it might be better to create a wrapper class instead of including it as a field in the Pydantic model.
(However, it must be ensured that the wrapper class is always unwrapped to apply the Pydantic model to the HTTP response, which can be enforced through middleware or similar mechanisms.)

Also, please do not modify the existing model to avoid affecting the current behavior.

We can solve this issue by using new @api_handler included in #3511

HyeockJinKim · 2025-01-20T14:57:48Z

src/ai/backend/manager/api/utils.py

 class BaseResponseModel(BaseModel):
-    status: Annotated[int, Field(strict=True, exclude=True, ge=100, lt=600)] = 200


If you want to specify the status_code, it might be better to create a wrapper class instead of including it as a field in the Pydantic model.
(However, it must be ensured that the wrapper class is always unwrapped to apply the Pydantic model to the HTTP response, which can be enforced through middleware or similar mechanisms.)

HyeockJinKim · 2025-01-20T14:58:37Z

src/ai/backend/manager/api/utils.py

 class BaseResponseModel(BaseModel):
-    status: Annotated[int, Field(strict=True, exclude=True, ge=100, lt=600)] = 200


Also, please do not modify the existing model to avoid affecting the current behavior.

HyeockJinKim · 2025-01-22T10:34:14Z

src/ai/backend/manager/api/vfolders/protocols.py

+class VFolderHandlerProtocol(Protocol):
+    async def create_vfolder(
+        self, request: web.Request, params: VFolderCreateRequestModel
+    ) -> VFolderCreateResponseModel: ...
+
+    async def list_vfolders(
+        self, request: web.Request, params: VFolderListRequestModel
+    ) -> VFolderListResponseModel: ...
+
+    async def rename_vfodler(
+        self, request: web.Request, params: VFolderRenameRequestModel
+    ) -> CreatedResponseModel: ...
+
+    async def delete_vfolder(
+        self, request: web.Request, params: VFolderDeleteRequestModel
+    ) -> NoContentResponseModel: ...


It seems that the definition of VFolderHandlerProtocol is not necessary.

Okay, I will delete it

HyeockJinKim · 2025-01-22T10:35:31Z

src/ai/backend/manager/api/vfolders/types.py

+
+
+class VFolderListRequestModel(BaseModel):
+    all: bool = Field(default=False)


Is all an existing flag?

all flag exists, but it seems deprecated option. Can I delete this flag in refactored APIs?

@check_api_params( t.Dict({ t.Key("all", default=False): t.ToBool, }), ) async def list_folders(request: web.Request, params: Any) -> web.Response: ... if params["all"]: raise InvalidAPIParameters("Deprecated use of 'all' option")

HyeockJinKim · 2025-01-22T10:36:27Z

src/ai/backend/manager/api/vfolders/types.py

+class VFolderListItem:
+    id: str
+    name: str
+    quota_scope_id: str
+    host: str
+    usage_mode: VFolderUsageMode
+    created_at: str
+    permission: VFolderPermission
+    max_size: int
+    creator: str
+    ownership_type: VFolderOwnershipType
+    user: str | None
+    group: str | None
+    cloneable: bool
+    status: VFolderOperationStatus
+    is_owner: bool
+    user_email: str
+    group_name: str
+    type: str  # legacy
+    max_files: int
+    cur_size: int


Is it possible to group the values among the fields?
If it’s difficult right now, we can proceed with it after implementing the service layer

I think we can proceed it when implementing service layer(we could do now, but it will change in high possibility when implementing service layer)

HyeockJinKim · 2025-01-22T10:37:16Z

src/ai/backend/manager/api/vfolders/types.py

+class VFolderListResponseModel(BaseResponseModel):
+    root: list[VFolderListItem] = Field(default_factory=list)
+
+    @classmethod
+    def from_dataclass(cls, vfolder_list: VFolderList) -> "VFolderListResponseModel":
+        return cls(root=vfolder_list.entries)
+


In my opinion, files or packages dealing with request and response should be separated from those dealing with internal models.

I separated api_schemas.py for req/res and dtos.py for the dataclasses

seedspirit self-assigned this Jan 17, 2025

github-actions bot added comp:manager Related to Manager component size:XL 500~ LoC area:docs Documentations labels Jan 17, 2025

HyeockJinKim reviewed Jan 17, 2025

View reviewed changes

HyeockJinKim reviewed Jan 22, 2025

View reviewed changes

seedspirit marked this pull request as ready for review February 4, 2025 14:15

seedspirit force-pushed the refactor/implemet-crud-handlers-for-manager branch from b910b73 to 471a16d Compare February 5, 2025 04:21

github-actions bot added the comp:common Related to Common component label Feb 5, 2025

HyeockJinKim changed the base branch from main to feat/add-pydantic-handling-decorator-for-req-res February 5, 2025 08:17

HyeockJinKim changed the base branch from feat/add-pydantic-handling-decorator-for-req-res to main February 5, 2025 08:18

seedspirit changed the base branch from main to feat/add-pydantic-handling-decorator-for-req-res February 5, 2025 08:36

seedspirit changed the base branch from feat/add-pydantic-handling-decorator-for-req-res to main February 5, 2025 09:35

seedspirit force-pushed the refactor/implemet-crud-handlers-for-manager branch from 30740cb to b000898 Compare February 5, 2025 09:47

github-actions bot added comp:agent Related to Agent component comp:client Related to Client component comp:cli Related to CLI component comp:webserver Related to Web Server component require:db-migration Automatically set when alembic migrations are added or updated labels Feb 5, 2025

seedspirit changed the base branch from main to feat/add-pydantic-handling-decorator-for-req-res February 5, 2025 09:48

seedspirit force-pushed the feat/add-pydantic-handling-decorator-for-req-res branch from 43510a5 to b1dce6c Compare February 5, 2025 09:50

feat: Apply api handler in vfolder handler

1eece08

seedspirit force-pushed the refactor/implemet-crud-handlers-for-manager branch from b000898 to 1eece08 Compare February 5, 2025 09:53

refactor: Split api_shema.py into request and response

b44bc22

seedspirit removed comp:agent Related to Agent component area:docs Documentations require:db-migration Automatically set when alembic migrations are added or updated labels Feb 5, 2025

seedspirit removed comp:webserver Related to Web Server component comp:common Related to Common component comp:client Related to Client component comp:cli Related to CLI component labels Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(BA-501): Add draft vFolder handler Interface of manager #3493

refactor(BA-501): Add draft vFolder handler Interface of manager #3493

seedspirit commented Jan 17, 2025 •

edited by github-actions bot

Loading

HyeockJinKim Jan 17, 2025

seedspirit Jan 17, 2025 •

edited

Loading

HyeockJinKim Jan 20, 2025

HyeockJinKim Jan 20, 2025

seedspirit Feb 4, 2025

HyeockJinKim Jan 20, 2025

HyeockJinKim Jan 20, 2025

HyeockJinKim Jan 22, 2025

seedspirit Feb 4, 2025

HyeockJinKim Jan 22, 2025

seedspirit Feb 4, 2025

HyeockJinKim Jan 22, 2025

seedspirit Feb 5, 2025

HyeockJinKim Jan 22, 2025

seedspirit Feb 5, 2025

		class BaseResponseModel(BaseModel):
		status: Annotated[int, Field(strict=True, exclude=True, ge=100, lt=600)] = 200



		class VFolderListRequestModel(BaseModel):
		all: bool = Field(default=False)

refactor(BA-501): Add draft vFolder handler Interface of manager #3493

Are you sure you want to change the base?

refactor(BA-501): Add draft vFolder handler Interface of manager #3493

Conversation

seedspirit commented Jan 17, 2025 • edited by github-actions bot Loading

Choose a reason for hiding this comment

seedspirit Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seedspirit commented Jan 17, 2025 •

edited by github-actions bot

Loading

seedspirit Jan 17, 2025 •

edited

Loading