Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

feat: Migrate filebrowsers to storage proxy #524

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

leksikov
Copy link
Contributor

Add new APIs to map the new manager-facing storage-proxy APIs for file browser.

@leksikov leksikov added this to the 21.03 milestone Jan 21, 2022
@leksikov leksikov self-assigned this Jan 21, 2022
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Patch coverage has no change and project coverage change: -0.07 ⚠️

Comparison is base (ab8b658) 48.63% compared to head (5608e6b) 48.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   48.63%   48.56%   -0.07%     
==========================================
  Files          53       53              
  Lines        9126     9126              
==========================================
- Hits         4438     4432       -6     
- Misses       4688     4694       +6     
Impacted Files Coverage Δ
src/ai/backend/manager/server.py 57.28% <ø> (-0.25%) ⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leksikov leksikov requested a review from achimnol January 25, 2022 08:10
@leksikov leksikov marked this pull request as ready for review January 25, 2022 08:10
@leksikov
Copy link
Contributor Author

Implemented the API to facilitate communication between client and Storage Proxy for file browser session.
There is an issue to think about. When user add his vfolder names how the manager can know which one belongs to which host and volune? Maybe user should also supply host and volume information?

@achimnol achimnol changed the title Feature/filebrowser in storage proxy feat: Migrate filebrowsers to storage proxy Feb 14, 2022
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

manager/server.py seems to be passed through black & isort.
Currently we are not modifying existing codebases at once along with a specific feature PR, so please make a separate PR to update code styles and revert the changes caused by black and isort in this PR.
I'd recommend using project-specific (venv-specific) lint configurations if you are using a global configuration.

default_cors_options: CORSOptions,
) -> Tuple[web.Application, Iterable[WebMiddleware]]:
app = web.Application()
app["prefix"] = "browser"
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename the URL prefix of this app module to storage/filebrowser.

)
folder_id = await conn.scalar(query)

query = sa.delete(vfolders).where(vfolders.c.id == folder_id)
Copy link
Member

Choose a reason for hiding this comment

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

This query is not executed. If missed, please add another await conn.execute().
Also, if you use different types of queries in a single scope (e.g., select & delete), please name the variables differently (e.g., select_query, delete_query) because in the future SQLAlchemy v2 with mypy extensions will check the different typing of query objects.



async def get_volume(root_ctx: RootContext, vfid: str) -> str:
async with root_ctx.db.begin() as conn:
Copy link
Member

Choose a reason for hiding this comment

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

If the transaction only includes read-only operations (e.g., select), use begin_readonly() instead of begin().

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants