Skip to content
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

feat(BA-624): Add vfolder repository layer in manager #3669

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

seedspirit
Copy link
Contributor

@seedspirit seedspirit commented Feb 12, 2025

resolves #3558 (BA-624)

Implemented repository layer for vfolder api in manager. Repository layer use database_engine (ExtendedAsyncSAEngine) created in /manager/models/utils

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • 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--3669.org.readthedocs.build/en/3669/


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

@github-actions github-actions bot added comp:manager Related to Manager component comp:common Related to Common component size:XL 500~ LoC labels Feb 12, 2025
@github-actions github-actions bot added the area:docs Documentations label Feb 12, 2025
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repository layer contains too much service logic.
@HyeockJinKim We should discuss the RBAC interface and implement it before proceeding with this work.

src/ai/backend/common/dto/manager/request.py Outdated Show resolved Hide resolved
src/ai/backend/common/dto/manager/request.py Outdated Show resolved Hide resolved
Comment on lines +47 to +55
async def get_group_type(self, group_id: uuid.UUID) -> ProjectType:
async with self._db.begin_session() as sess:
query = sa.select(GroupRow).where(GroupRow.id == group_id)
group_row = await sess.scalar(query)

if group_row is None:
raise GroupNotFound(extra_data=group_id)

return group_row.type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a project querying function in the VFolderRepository class?
I understand this function might be needed to check permissions of vfolders related to group types. Please add comments explaining the implementation rationale and provide guidance for future contributors. e.g. Relocate this function to ProjectRepository etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to the vFolderRepository because the existing vfolder create API operation used that behavior. For now, how about leave it in this repository for now and move it when another repository is created, perhaps as a TODO comment for future contributor?

Comment on lines +57 to +61
async def get_user_container_id(self, user_id: uuid.UUID) -> Optional[int]:
async with self._db.begin_session() as sess:
query = sa.select(UserRow.container_uid).where(UserRow.uuid == user_id)
result = await sess.scalar(query)
return result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also same here. I added this method cause it was part of vfolder create API. As this PR is the first PR for introducing repository layer in vfolder API, how about leave it in this repository for now and move it when another repository is created, perhaps as a TODO comment for future contributor?

src/ai/backend/manager/api/vfolders/repositories.py Outdated Show resolved Hide resolved
src/ai/backend/manager/api/vfolders/repositories.py Outdated Show resolved Hide resolved
Comment on lines +130 to +138
async def persist_vfolder_metadata(self, metadata: VFolderMetadataToCreate) -> VFolderItem:
async with self._db.begin_session() as sess:
insert_query = sa.insert(VFolderRow).values(metadata.to_dict()).returning(VFolderRow.id)
vfolder_id = await sess.scalar(insert_query)

query = sa.select(VFolderRow).where(VFolderRow.id == vfolder_id)
vfolder: VFolderRow = await sess.scalar(query)
vfolder_item = VFolderItem.from_orm(orm=vfolder, is_owner=True)
return vfolder_item
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It is good to wrap a DB write execution into execute_with_txn_retry().
  2. Just a question, why is the name of this method "persist_...()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the vfolder needs to complete requests to the storage-proxy, etc. to be fully created, I thought it would be more accurate to say that the vfolder was 'persisted' to the DB in a more strict sense rather than 'added' or 'created'

src/ai/backend/manager/api/vfolders/repositories.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:common Related to Common component comp:manager Related to Manager component size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Repository Layer for vFolder CRUD APIs in Manager
2 participants