-
Notifications
You must be signed in to change notification settings - Fork 76
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
docs: fsspec documentation #1074
base: main
Are you sure you want to change the base?
Conversation
The new docs/readthedocs.org:uproot test requirement can be satisfied by merging #1084/jpivarski/fix-readthedocs-documentation into this PR (or by merging in |
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.
Thanks for writing this up! Overall, this is covering the issues, but it's doing so from a developer's point of view, not a user's point of view. Moreover, the readers of this documentation (basic.rst) are often first-time users; the reference documentation in docstrings/on individual pages are for people who already know what they're doing.
The inline comments below are mostly asking for things to be removed, and only in the introduction. The individual-feature sections are great as-is.
Since version `5.2.0 <https://github.com/scikit-hep/uproot5/releases/tag/v5.2.0>`_, uproot supports reading and writing files using `fsspec <https://filesystem-spec.readthedocs.io/en/latest/>`_. | ||
This allows you to read and write files from a variety of sources, including cloud storage, HTTP, and more. |
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.
Not really needed—readers of documentation ought to presume that it describes the most recent version of the code. (/latest/
is in the URL.)
Usage of fsspec as a source is the default behaviour since 5.2.0, but the user is able to manually specify the source by passing a `uproot.source.chunk.Source` class to the `handler` argument of different uproot methods, such as `uproot.open`, `uproot.iterate`, `uproot.concatenate`, etc. | ||
|
||
In general the user should not need to worry about the source, as uproot will automatically choose the best source for the given path. |
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.
If users don't need to worry about it, then it doesn't make sense to bring it up here. In the diátaxis taxonomy, this basic.rst page is in the tutorials/learning corner:
So anything detailed can be relegated to the docstrings (the information/reference corner).
|
||
In general the user should not need to worry about the source, as uproot will automatically choose the best source for the given path. | ||
|
||
In some cases it may provide a performance benefit to manually specify the source, for example when opening a file from a local path, specifying `handler=uproot.source.file.MemmapSource` (instead of the default `handler=uproot.source.fsspec.FSSpecSource`) may reduce the time to open the file at the cost of using more memory. |
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.
This sort of thing would be better to address in GitHub Discussions, after it comes up. These are the sorts of concerns that only matter at scale, and some users worry about all of these things because they've read about it, but their file is only a few MB in size.
Any fsspec protocol should work for reading, while only the protocols supporting writing will work for writing. | ||
|
||
fsspec is a dependency of uproot, but in order to use some protocols, the user may need to install additional dependencies. | ||
For example, in order to open S3 files, the user needs to have `s3fs <https://github.com/fsspec/s3fs>`_ installed. | ||
When attempting to open a file with a protocol that is not supported, uproot will raise an exception with a helpful message pointing towards the missing dependency. | ||
|
||
For some protocols, such as `s3` or `ssh`, fsspec may need additional options, such as credentials. These can be directly passed as keyword arguments to the uproot function, and will be passed to fsspec. |
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.
This is good information, though it should come after a basic "how to" sentence. The first thing readers of this documentation should see is a statement saying that they can use any remote protocol that fsspec (link) knows about by writing a URL schema, such as "https://
", "root://
", or "s3://
".
Second, you'd say that they might be prompted to load additional dependencies.
Third, you'd say that some of these protocols will work for writing; see the fsspec documentation for details.
Fourth, that some of them require additional arguments, and they can be passed as keyword **options
.
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.
The comment on writing could also indicate that this is possible, but likely inefficient. The best way to write a file and host it remotely is to write it locally and upload the final result.
|
||
For some protocols, such as `s3` or `ssh`, fsspec may need additional options, such as credentials. These can be directly passed as keyword arguments to the uproot function, and will be passed to fsspec. | ||
|
||
Keep in mind that there might be different libraries that implement a given fsspec backend. This might lead to errors when using uproot. For example, the fsspec ssh tests assume `paramiko <https://github.com/paramiko/paramiko>`_ is installed, but another library such as `sshfs <https://github.com/fsspec/sshfs>`_ might be present instead which also adds ssh support but might behave differently. |
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.
This should be dealt with in GitHub Discussions or Issues, when users run into it.
(I'm assuming that the majority won't, and the tutorial documentation has to be streamlined for the majority/)
@@ -1251,3 +1251,92 @@ In addition, each TBranch of the TTree can have a different compression setting: | |||
{'x': None, 'ny': None, 'y': ZLIB(4)} | |||
|
|||
Changes to the compression setting only affect TBaskets written after the change (with :ref:`uproot.writing.writable.WritableTree.extend`; see above). | |||
|
|||
Using fsspec for reading and writing files |
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.
Since this documentation should be user-oriented, we might even go so far as to not call this section "Using fsspec," but "File access through remote protocols." In the text, you can say that we use fsspec to do it, and therefore direct users to fsspec if they need details. But the section header should be useful for people who want to read or write remote files and don't know how, or don't even know if Uproot can do it.
Update docs with fsspec integration usage.