-
Notifications
You must be signed in to change notification settings - Fork 368
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(gist): fsspec file system for GitHub gists (resolves #888) #1791
base: master
Are you sure you want to change the base?
Conversation
Thanks for providing! I haven't had a chance to look yet, but I will soon :) |
Most welcome, no worries! 😃 |
Quick suggestion: it would be good to enable bundling the gist ID with the URL:
like github: allows. It would require enabling extracting kwargs from the URL. |
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.
As I read through, I found that you answered almost all of my questions elsewhere in the code :)
I don't think there's a good way to test this thoroughly, but at least we can reasonably expect gist to be available whenever GHA is running.
import fsspec | ||
|
||
|
||
@pytest.mark.parametrize("gist_id", ["16bee4256595d3b6814be139ab1bd54e"]) |
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.
Perhaps a good idea to pin to a specific hash, so that the values can be tested.
r.raise_for_status() | ||
return MemoryFile(path, None, r.content) | ||
|
||
def cat(self, path, recursive=False, on_error="raise", **kwargs): |
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 surprised this doesn't work already with the parent version in AbstractFileSystem
Parse 'gist://' style URLs into GistFileSystem constructor kwargs. | ||
For example: | ||
gist://:TOKEN@<gist_id>/file.txt | ||
gist://username:TOKEN@<gist_id>/file.txt |
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.
Does this already allow for my suggestion that fsspec.open() should work in one go? It looks like it should. One more thing we might want to care about here, is the commit hash - do you think it can be added, or does it need to be kwarg-only?
GitHub username for authentication (required if token is given). | ||
token : str (optional) | ||
GitHub personal access token (required if username is given). | ||
timeout : (float, float) or float, optional |
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.
Perhaps more general set of arguments to pass to requests?
self.timeout = timeout if timeout is not None else (60, 60) | ||
|
||
# We use a single-level "directory" cache, because a gist is essentially flat | ||
self.dircache[""] = self._fetch_file_list() |
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.
It is unusual to fetch file listings on init, but I think it makes sense in this case.
Clear the dircache. If path is given, we could refetch—but for gist, | ||
we typically refetch everything in one shot anyway. | ||
""" | ||
self.dircache.clear() |
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.
Interesting way to "refresh". There could be a refresh= kwarg on ls(); but actually I'd be happy to treat the dircache as immutable for the purposes of this implementation.
Please ping me when I should have another look |
Thanks for reviewing Martin, gotten sidetracked in a CI fixing rabbit hole this week I've thankfully emerged and can return to revisit this!
Will do 🫡 |
This PR introduces a new filesystem backend,
GistFileSystem
, which allows read-only access to files within a single GitHub Gist (as suggested in #888). I'd find this really useful in combination with Universal Pathlib (also an fsspec project)!GithubFileSystem
but simplified for a single gist.Users can do:
For a private gist, the same but also passing
username
andtoken
args.ls
,_open
,cat
,invalidate_cache
), read-only impldocs/source/api.rst
.Example usage
Below is a short snippet showing how to retrieve files from a public gist:
⇣