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

Set up backend structure and list all available drives #3

Merged
merged 29 commits into from
Nov 20, 2023

Conversation

DenisaCG
Copy link
Member

@DenisaCG DenisaCG commented Oct 30, 2023

Set up the backend structure.

Created managers as an abstraction layer to enable extensibility, namely for the providers. For this purpose, a base manager class JupyterDrivesManager was created, and the corresponding parent class S3Manager. The entrypoints package is also used to list providers for the managers.

A base handler class was also instated, as JupyterDrivesAPIHandler. All other handlers implement their logic on top of it. To make it as simple and extensible as possible, the handlers and endpoint responses are meant to be provider independent.

In base.py, we handle the provider selection based on jupyter_notebook_config.py, through the DrivesConfig class. The user can set a session token, a secret access token and an access key id to enable access to the desired S3 drives.

@github-actions
Copy link

Binder 👈 Launch a Binder on branch DenisaCG/jupyter-drives/listAvailableDrives

@DenisaCG DenisaCG added the enhancement New feature or request label Nov 6, 2023
@DenisaCG DenisaCG self-assigned this Nov 6, 2023
Copy link
Contributor

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @DenisaCG this looks very promising

jupyter_drives/handlers.py Outdated Show resolved Hide resolved
jupyter_drives/handlers.py Outdated Show resolved Hide resolved
jupyter_drives/handlers.py Outdated Show resolved Hide resolved
jupyter_drives/managers/manager.py Outdated Show resolved Hide resolved
# """
# raise NotImplementedError()

async def _call_provider(
Copy link
Contributor

Choose a reason for hiding this comment

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

We may not be as lucky as the pull requests extension about this. But let's see how it goes in the future.

Copy link
Contributor

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @DenisaCG

I have some minor suggestion, but it looks really great.

jupyter_drives/base.py Outdated Show resolved Hide resolved
jupyter_drives/base.py Outdated Show resolved Hide resolved
jupyter_drives/base.py Outdated Show resolved Hide resolved
jupyter_drives/handlers.py Outdated Show resolved Hide resolved
jupyter_drives/handlers.py Outdated Show resolved Hide resolved
jupyter_drives/managers/s3.py Outdated Show resolved Hide resolved
jupyter_drives/tests/test_handlers.py Outdated Show resolved Hide resolved
@DenisaCG
Copy link
Member Author

Thanks for the review and suggestions @fcollonval!

@DenisaCG DenisaCG changed the title WIP Listing all available drives Set up backend structure and list all available drives Nov 20, 2023
@DenisaCG DenisaCG merged commit 878c178 into QuantStack:main Nov 20, 2023
7 checks passed
@DenisaCG DenisaCG deleted the listAvailableDrives branch November 20, 2023 15:56
Copy link
Contributor

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

@DenisaCG I have some post mortem comment. If you could take them into account in a follow-up PR it will be great


# When
response = await jp_fetch("jupyter-drives", "drives")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the error code if the credentials are invalid? Do we get an error code like 401 that could in the future be used in the frontend to notify the user credentials are invalid?

tsconfig.json Show resolved Hide resolved
jupyter_drives/managers/s3.py Show resolved Hide resolved
jupyter_drives/managers/s3.py Show resolved Hide resolved
@DenisaCG
Copy link
Member Author

Thanks for the additional comments, @fcollonval! All of this has been improved in #9 (the credentials errors are still a work in progress, but will be pushed to the same PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants