-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
[WIP] New widget : RemoteFileSelector #6301
Conversation
…rrotsmnrd/main
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6301 +/- ##
===========================================
- Coverage 82.74% 71.69% -11.06%
===========================================
Files 301 303 +2
Lines 45248 45433 +185
===========================================
- Hits 37442 32573 -4869
- Misses 7806 12860 +5054
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Really nice, will review this week! |
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.
I know the PR is still in draft mode, but I have left some comments.
You don't need to answer them before the PR is ready to review.
raise NotImplementedError() | ||
|
||
# for S3RemoteFileProvider | ||
import s3fs |
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.
There is no need to make this a required dependency.
As far as I can, see this can be put into a if TYPE_CHECKING
def __init__(self): | ||
super().__init__() |
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.
def __init__(self): | |
super().__init__() | |
def __init__(self, **params): | |
super().__init__(**params) |
Otherwise, parameters from inherited classes will not work.
(The __init__
is not needed here, but since this PR is still in draft, things can change)
self.buckets = buckets | ||
self.file_pattern = file_pattern | ||
|
||
async def ls(self, path:PurePosixPath): |
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.
I'm curious why this is async with no await inside it.
super().__init__() | ||
|
||
async def ls(self, path:PurePosixPath): | ||
time.sleep(1) |
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.
time.sleep(1) | |
asyncio.sleep(1) |
So it makes sense with async.
|
||
# Set up state | ||
self._stack = [] | ||
self._cwd = PurePosixPath("/") |
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.
Is it always the case it is a PosixPath?
There could be a RemoteFileProvider that connects to a Windows Server.
My main question surrounds |
Extending to Google Drive would be great as well (or is that what is meant by gcfs)? Google Drive has a Python API with OAuth 2, but I guess that it is provider specific logic. But isn't that unavoidable in these cases, to enable remote file access to S3, Google Drive, Microsoft, etc. that are widely used? Would definitely be useful to have this though. Isn't there a python library somewhere that can access multiple remote filesystems that can be leveraged? So a (well-maintained) python equivalent to something like this? https://github.com/rclone/rclone?tab=readme-ov-file |
Or start with the Top 3 besides S3 if that is already there? So Google Drive, Microsoft and ... |
Related to this PR ? |
Recent reported working example of using fsspec.gui.FileSelector to access remote files in a Notebook with screenshot: fsspec/community#9 Could this be an already existing solution for this issue @philippjfr ? Especially because fsspec supports many different filesystems. |
This was super helpful as a starting point, thanks @pierrotsmnrd. I've picked this up and integrated the remote file support in the regular |
Hi @pierrotsmnrd . Great work! You mentioned the below document, but I can't find it in the Panel repo or in your fork. Do you have a link to the doc?
|
This PR is a work in progress
Purpose
This PR introduces a new widget : RemoteFileSelector
It's inspired by and based on the standard panel
FileSelector
, but its goal is to display a remote filesystem.RemoteFileSelector is only responsible for showing the data, and selecting files/dirs. It relies on a RemoteFileProvider that is responsible for fetching the data from the remote filesystem.
This PR contains S3RemoteFileProvider, used to display data from a S3 Bucket via a RemoteFileProvider.
In the future, we can imagine other file providers to fetch data from a Github repo, a FTP server, etc.
Example
This example, given a proper
.env
file, yields a result similar to :Remaining to do :
DummyRemoteFileProvider
for this purpose inpanel/tests/widgets/test_remote_file_selector.py
. I need to write a proper test using itexample/reference/widgets/RemoteFileSelector.ipynb
I can use some help, guidance and advice, for these two points.