feat: globbing with fsspec

[lobis]

Implement globbing support using fsspec.

Currently not working for all protocols due to fsspec/filesystem_spec#1459. In Related to scikit-hep/fsspec-xrootd#40 we implement the necessary changes to make it work for xrootd.

A new release of fsspec-xrootd is necessary for this to work. (DONE!)

Before this PR, glob expressions were ignored for all but local file paths.

This PR tries to expand them using fsspec, but this may not always work due to fsspec/filesystem_spec#1459. We made sure it worked for xrootd. Not working here means that the glob expression will be expanded but the full path will be wrong due to the protocol prefix being incomplete (missing auth information, etc.). uproot will just throw an exception as if it tried to open a bad path.

Added tests for xrootd and s3 globs (http glob does not work at the moment).

In order to correclty expand the glob expression fsspec may need additional storage_options. These are part of the options directionary that needs to be passed to the regularize_files method.

[lgray]

fsspec-xrootd 0.2.3 that includes scikit-hep/fsspec-xrootd#40 is now on pypi

[jpivarski]

The Windows tests are failing because it's thinking that the drive letter is a URI scheme:

        if isinstance(files, str):
            if parse_colon:
                file_path, object_path = file_object_path_split(files)
            else:
                file_path, object_path = files, None
    
            parsed_url = urlparse(file_path)
            scheme = parsed_url.scheme
            if scheme in fsspec.available_protocols():
                # user specified a protocol, so we use fsspec to expand the glob and return the full paths
                file_names_full = [file.full_name for file in fsspec.open_files(files)]
                # https://github.com/fsspec/filesystem_spec/issues/1459
                # Not all protocols return the full_name attribute correctly (if they have url parameters)
                for file_name_full in file_names_full:
                    yield file_name_full, object_path, maybe_steps
            elif scheme != "":
                # user specified a protocol, but it's not supported by fsspec (e.g. user does not have s3fs installed)
>               raise ValueError(
                    f"Protocol {scheme} is not supported by fsspec. Please install the corresponding package."
                )
E               ValueError: Protocol c is not supported by fsspec. Please install the corresponding package.

HasBranches = <class 'uproot.behaviors.TBranch.HasBranches'>
counter    = [1]
file_path  = 'C:\\Users\\runneradmin\\.local\\skhepdata\\uproot-Zmumu.root'
files      = 'C:\\Users\\runneradmin\\.local\\skhepdata\\uproot-Zmumu.root:events'
files2     = 'C:\\Users\\runneradmin\\.local\\skhepdata\\uproot-Zmumu.root:events'
maybe_steps = None
object_path = 'events'
parse_colon = True
parsed_url = ParseResult(scheme='c', netloc='', path='\\Users\\runneradmin\\.local\\skhepdata\\uproot-Zmumu.root', params='', query='', fragment='')
scheme     = 'c'
steps_allowed = True

The same preprocessing that protects "open file" should be applied to "expand glob."

[jpivarski]

It looks like you just got it in 50835ae.

[jpivarski]

lobis changed the title feat: globbing with fsspec (xrootd only) feat: globbing with fsspec

Does that mean that the new fsspec is out? No, fsspec/filesystem_spec#1459 hasn't been merged, and Martin has a response about it.

[jpivarski]

It looks like it only tests globbing in XRootD, so I'll assume that's what it does.

[jpivarski]

When the tests pass, this will be a good addition, adding globbing for XRootD. (So far, XRootD only.)

If this is opening the door to globbing with any URI scheme and the set of schemes that work depends on what gets implemented in fsspec, that's fine. We can refer users to fsspec if they're wondering why it works in XRootD and not in HTTP. That's part of the separation of concerns we're setting up between Uproot and XRootD.

[lgray]

Looks like this one needs its failed tests rerun as well. Failures appear unrelated.

[jpivarski]

I wonder if this is one of the tests that had previously been called "super-flaky"? @lobis reinstated quite a few of these and they hadn't been acting up, maybe until now. The issue is an XRootD connection failure.

Maybe we want to install this: https://github.com/marketplace/actions/retry-step

(Maybe also here: https://github.com/CoffeaTeam/integration-test/blob/main/.github/workflows/test.yml)

[lgray]

Since this one is approved - if it can go in 5.2.0 please add it :-)

[lobis]

I wonder if this is one of the tests that had previously been called "super-flaky"? @lobis reinstated quite a few of these and they hadn't been acting up, maybe until now. The issue is an XRootD connection failure.

Maybe we want to install this: https://github.com/marketplace/actions/retry-step

(Maybe also here: https://github.com/CoffeaTeam/integration-test/blob/main/.github/workflows/test.yml)

Yes the xrootd tests are the most problematic and likely to fail due to server issues.

We are already doing reruns for the xrootd tests, with the following settings: --reruns 3 --reruns-delay 30, so I'm not sure about adding this retry-step action, it feels redundant (maybe I didn't understand what it does). Perhaps we should increase the number of reruns and the time between them.

[lobis]

It looks like it only tests globbing in XRootD, so I'll assume that's what it does.

Actually this should work for all fsspec filesystems that support globbing. I added some tests for s3 to show this. http will not work (I added a test that should fail if fsspec ever implements globbing for http, so we are noticied).

Since these tests expand a glob expression they may fail in the future if some other files are added that match the glob. I set the tests in such a way that this is hopefully easy to spot and fix.

[jpivarski]

Right, let's not add multiple ways to retry. But if the error rate is high enough that with 3 retries we're still seeing it, then perhaps the retry parameters should get increased: --reruns 5 --reruns-delay 60 (I assume that's 60 seconds).

That doesn't need to be done for this PR, though, which has nothing to do with the failing tests. For this PR, we can "rerun failed tests" manually go get all the green checkmarks and then merge the PR when everything is verified-okay.