feat: fsspec for all non-object writing - %-encoded urls no longer decoded #1034

lobis · 2023-11-17T03:54:30Z

Make all writing go through fsspec (when requesting to write to a urlpath, if it's a file-like object, then fsspec doesn't apply).
When uproot.create fails due to a file already existing, a FileExistsError (derived from OSError) will be thrown instead of a generic OSError
Simplified how the source class is chosen based on the urlpath
Remove windows specific code and test (no longer makes sense imo). I tried to cover the same cases (uri in path) in new test functions in the fsspec writing tests. Since these tests write local files and also run in windows, they automatically parse the full uri with the drive letter without issue so no need to explicitly test it. Improper parsing of a file:// uri on Windows #325

How urlpath strings are processed now:

Uproot is only responsible for stripping the object if specified (under the path:object notation). Then the path is passed to fsspec. fsspec is responsible of correctly resolving the path).

I updated the current tests to cover all cases in #325 regarding windows paths and all it's variations. I had to delete the older test file and include these tests in another file because the path resolution is now done by fsspec so we cannot test it before opening the file.

All previous cases appear to be covered by fsspec.

There is one breaking change however:

Before this PR, uproot was decoding %-encoded urls when they had the file:// scheme. Because now everything goes through fsspec it's not a good idea to distinguish between having a file:// or not (fsspec will infer the scheme in case it's not present, which will correspond to file://).

Now we are not decoding %-encoded urls, they are passed as they come to fsspec (and fsspec won't decode them either). Now it is the user's responsibility to decode these urls should they want to.

…object and fsspec)

jpivarski

Are you sure this covers all the same cases as tests/test_0325_fix_windows_file_uris.py?

jpivarski · 2023-11-18T02:12:24Z

tests/test_0692_fsspec_reading.py

+        ("file.root", uproot.source.fsspec.FSSpecSource),
+        ("s3://path/file.root", uproot.source.fsspec.FSSpecSource),
+        (r"C:\path\file.root", uproot.source.fsspec.FSSpecSource),
+        (r"file://C:\path\file.root", uproot.source.fsspec.FSSpecSource),


This is an example with a drive letter and a file://, but what about a drive letter without file://, or with file:, or file:///, or backward versus forward slashes? A lot of these things are considered legal (anything that could be copy-pasted into a URL bar in Edge).

This is an example with a drive letter and a file://, but what about a drive letter without file://, or with file:, or file:///, or backward versus forward slashes? A lot of these things are considered legal (anything that could be copy-pasted into a URL bar in Edge).

The reason I removed the test file is because we no longer do a processing of the path, we just split the object from the path if it exists, then feed the path directly to fsspec. fsspec should be smart enough to handle these different cases.

Now that I think about it this is only true if fsspec source is the one to be used, if the user manually selects some other source, we are losing this processing... I think the goal here is to let fsspec do all processing, if fsspec is not able to resolve some path and we think it should, we should open an issue in fsspec. We are only responsible of transforming the non-compliant urlpath:object into a compliant urlpath (that's my idea atleast).

With this approach things such as file:///g:/mydir/file.root are not going to work. They would get interpreted as /g:/mydir/file.root which is not a valid windows path... (I think) should they work, though? My browser also interprets it as /g:/mydir/file.root. However on issue #325 it's stated as a valid windows path.

Also the previous implementation treats non-schema paths and paths with file:// differently, which I don't really understand. fsspec treats paths without schema as local filesystem paths: fsspec.core.url_to_fs("file:///tmp/file.root") and fsspec.core.url_to_fs("/tmp/file.root") are the same. But different from fsspec.core.url_to_fs("file://tmp/file.root") which assumes tmp/file.root is a non-absolute path, same as fsspec.core.url_to_fs("file:tmp/file.root") or fsspec.core.url_to_fs("tmp/file.root").

My point is: should uproot be smarter than fsspec when resolving paths? assuming the user passes a filepath (without specifying an object in the uproot-way), we should just pass it straight to fsspec without doing anything to it. If fsspec fails to resolve it, then the path is wrong or it isn't and the issue should be raised in fsspec, not uproot. Do you agree with this @jpivarski ? I think this is in line with the motivation of the fsspec integration: offload responsability from uproot.

Uproot should not be smarter than fsspec about resolving paths. That is fsspec's job. We should at least know what fsspec does, though. It may be that by handing this over to fsspec, some of our old tests no longer pass, and that's something that we can note in the test code as a reminder if it ever comes up. (This is something that users will bring up to us, since we don't use Windows, but some of them do.)

If fsspec doesn't accept these paths, we can also ask @martindurant why not. Maybe it hasn't been considered, or maybe file:g:\file.root is not satisfying any protocol and Uproot had been too loose in accepting it.

maybe file:g:\file.root is not satisfying any protocol

I think it would have to be
file://g:/file.root or file://g:\file.root or g:/file.root

I've only ever seen the single colon in terms of windows drive letters and the special case of data: URLs. fsspec considers protocols to be the thing before "::" (separating multiple protocol parts) or "://".

I agree.

To be clear: file:g:\file.root is treated as file://g:\file.root (g:\file.root without schema) by fsspec and is thus a valid path. This would still work.

file:/g:\file.root is treated as file:///g:\file.root (/g:\file.root without schema) which is not a valid windows path (according to #325). These are currently supported by uproot but wouldn't be after this PR if merged as of right now. #325 claims this is a valid windows path, but is it really?

We can lose some capabilities if we had just been too loose with it in the past. All I was asking for here was to know that we're losing them and why.

I had been taking "a web browser would accept it" as a way of defining good URIs. I think web browsers accept file:path/to/file (only for local paths, not http:path/to/file), though I can't personally test how this would interact with Windows drive letters.

So it may be that all this PR needs to be complete is an accounting of which patterns we now accept and which have been lost, on purpose.

lobis · 2023-11-20T18:08:07Z

Are you sure this covers all the same cases as tests/test_0325_fix_windows_file_uris.py?

I also wanted to ask about the %-encoded uris. As of this PR the decoding uproot does is removed. I'm not sure I understand the motivation behind this (decoding conditionally if the file:// scheme is present or not). I would be in favour of not doing the decoding and passing it as it stands to fsspec. Would there be any problem with this?

jpivarski · 2023-11-20T18:10:33Z

%-encoding is another thing that web browser location bars accept. I don't remember if that was the specific motivation here. It certainly would have come from a user request—I wouldn't have thought to add something like that without someone asking for it. Do you see, tracing back from the point when it was added, which issue prompted it, if any?

lobis · 2023-11-20T19:27:47Z

%-encoding is another thing that web browser location bars accept. I don't remember if that was the specific motivation here. It certainly would have come from a user request—I wouldn't have thought to add something like that without someone asking for it. Do you see, tracing back from the point when it was added, which issue prompted it, if any?

It appears to be triggered by the same issue #325 which was resolved by #328. I appears @jpivarski added the support for %-encoded as a "bonus" to #325. I think it may come from https://stackoverflow.com/questions/5977576/is-there-a-convenient-way-to-map-a-file-uri-to-os-path# (linked in the issue post).

lobis · 2023-11-20T20:19:32Z

@jpivarski it appears that the previous windows path test cases are still covered by fsspec (I updated the tests to cover them in https://github.com/scikit-hep/uproot5/pull/1034/files#diff-67e65d3e7efb19f80946512ff5b8aedad9c5dcc887febe2e0968348cfe8f14a5R34-R50).

The only breaking change is that we no longer decode %-encoded urls. I think this change makes sense since we should not distinguish between urlpaths having file:// or not (since this is handled by fsspec).

jpivarski · 2023-11-20T20:23:48Z

It seems reasonable to not decode %-encoded URLs. Go ahead with this change.

…coded (#1034) * writing goes through fsspec * increase rc version * type hints and docs * add helper methods, create * throw more specific error * add additional test for `create` failure with scheme other than local * simplify source selection * remove windows specific code * raise exception if invalid combination of handler / input (file-like object and fsspec) * use softer check for file-like object * cover problematic case with additional slash (file:///c:/file.root) * test "file:" scheme (no slash) * test backslash

* feat: add fsspec as required dependency (#1021) * fsspec requirements * simplify fsspec import * use loop property * correctly create schemes list * feat: set fsspec as default source (#1023) * feat: add fsspec as required dependency (#1021) * fsspec requirements * simplify fsspec import * use loop property * correctly create schemes list * remove deprecated handlers from docs * simplify source selection * return object source * pickle executor * rename test * test more handlers * option to check writeable file-like object * rename test * explicitly set handler * fix s3 source * rename test * Revert "fix s3 source" This reverts commit e76fdbb. * sesparate PR for s3 fix (#1024) * strip file:// * rename test * rename tests * add aiohttp skip * attempt to parse windows paths * test ci * Revert "test ci" This reverts commit 4c1c8a5. * rename test * remove fsspec from test * remove *_handler options * update defaults * do not override default s3 * do not use fsspec for multiprocessing * rename test * fix not selecting object source * missing import * normalize doc * remove helper * never return None as source * remove unnecessary xrootd source default override since fsspec is default now * rename test * add empty class to pass old pickle test * feat: set `fsspec` (`s3fs`) as default handler for s3 paths (#1032) * feat: add fsspec as required dependency (#1021) * fsspec requirements * simplify fsspec import * use loop property * correctly create schemes list * feat: set fsspec as default source (#1023) * feat: add fsspec as required dependency (#1021) * fsspec requirements * simplify fsspec import * use loop property * correctly create schemes list * remove deprecated handlers from docs * simplify source selection * return object source * pickle executor * rename test * test more handlers * option to check writeable file-like object * rename test * explicitly set handler * fix s3 source * rename test * Revert "fix s3 source" This reverts commit e76fdbb. * sesparate PR for s3 fix (#1024) * strip file:// * rename test * rename tests * add aiohttp skip * attempt to parse windows paths * test ci * Revert "test ci" This reverts commit 4c1c8a5. * rename test * remove fsspec from test * remove *_handler options * update defaults * do not override default s3 * do not use fsspec for multiprocessing * rename test * fix not selecting object source * missing import * normalize doc * remove helper * never return None as source * remove unnecessary xrootd source default override since fsspec is default now * rename test * add empty class to pass old pickle test * set version to 5.2.0rc1 (release candidate) * set s3fs as default for s3 * test different handlers * correct serialization of fsspec source * feat: simplify object path split (#1028) * simplify object path split * add example from #975 * fix tests * add more test cases * test case update * remove scheme unused regex * feat: fsspec for all non-object writing - %-encoded urls no longer decoded (#1034) * writing goes through fsspec * increase rc version * type hints and docs * add helper methods, create * throw more specific error * add additional test for `create` failure with scheme other than local * simplify source selection * remove windows specific code * raise exception if invalid combination of handler / input (file-like object and fsspec) * use softer check for file-like object * cover problematic case with additional slash (file:///c:/file.root) * test "file:" scheme (no slash) * test backslash * add new test case * split big test in two * retry on socket error * xrootd iterator * iterate over different files * iterate over tree * pytest fixture for test directory * pytest fixture for test directory * add annotation to open argument * remove repeated test