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

docs: fsspec documentation #1074

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

docs: fsspec documentation #1074

wants to merge 5 commits into from

Conversation

lobis
Copy link
Collaborator

@lobis lobis commented Dec 20, 2023

Update docs with fsspec integration usage.

@jpivarski
Copy link
Member

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 main, which has it).

@lobis lobis requested a review from jpivarski January 22, 2024 08:36
@lobis lobis marked this pull request as ready for review January 22, 2024 08:36
Copy link
Member

@jpivarski jpivarski left a 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.

Comment on lines +1258 to +1259
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.
Copy link
Member

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.)

Comment on lines +1261 to +1263
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.
Copy link
Member

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.
Copy link
Member

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.

Comment on lines +1267 to +1273
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.
Copy link
Member

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.

Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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.

@jpivarski jpivarski added the inactive A pull request that hasn't been touched in a long time label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive A pull request that hasn't been touched in a long time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants