fix(filesystem): UNC paths are not supported #1209

IlyaFaer · 2024-04-11T09:44:41Z

Towards #1175

netlify · 2024-04-11T09:44:58Z

✅ Deploy Preview for dlt-hub-docs canceled.

Name	Link
🔨 Latest commit	`556a256`
🔍 Latest deploy log	https://app.netlify.com/sites/dlt-hub-docs/deploys/662ce2b6dbd3a2000863046b

IlyaFaer · 2024-04-11T13:31:43Z

@rudolfix, that's a couple of cunning fixes. I'd say I didn't find ideological errors - it's just about a couple of symbols (normal ones, they work find in Python file opening), which make fsspec (not our code) to turn the path into something weird. Can't say I like it, but it worked. Pushed a test in verified-sources: dlt-hub/verified-sources#423

rudolfix

@IlyaFaer it is hard to say if this works or not without comments and tests or maybe I'm reviewing too early?

IlyaFaer · 2024-04-12T08:07:11Z

@rudolfix, I added the test into verified-sources (dlt-hub/verified-sources#423), to check it with filesystem. It passes locally:

And as I said, there is no mistake in the code. There's just a bunch of if in the fsspec, like if path.startswith("//") and not ":" in path. Because of these conditions met, fsspec treats a UNC path incorrectly and tries to make it absolute, adding an incorrect prefix to the path. The path itself is fine, just a bit chaotic with slashes. Python deals with it easily, however, fsspec can't chew it up. So, a small rearrange of slashes and colons in case of UNC is needed to make it pass through all the conditions without significant changes of the path.

Otherwise, UNC works fine with fsspec and filesystem. They just sometimes can't recognize that the path is UNC.

rudolfix

OK good!

but you can add tests also here. we are testing glob in test_local_fileststem. please unit test this

UNC path
windows path starting with drive letter
windows path using backslashes

rudolfix

more specific on the test, following https://en.wikipedia.org/wiki/File_URI_scheme
please test following windows paths

C:\a\b\b
\localhost\c$\a
file://server/folder/data.xml
file:////server/folder/data.xml (if the above works, you can skip this one)
with the glob function

rudolfix · 2024-04-12T12:22:31Z

dlt/common/storages/fsspec_filesystem.py

-            path=posixpath.join(bucket_url_parsed.path, file_name)
-        ).geturl()
+
+        if file.startswith("//"):


when do we get files starting with //? is this UNC path? please comment on that

Well, that's legacy, but I think we should consider it as equal to \\. Because... well, dealing with slash direction is very unpleasant for users.

Anyway, I don't think it's needed anymore. glob seems to be handling fine all the possible cases.

dlt/common/storages/fsspec_filesystem.py

IlyaFaer · 2024-04-16T08:45:55Z

@rudolfix, hm, I don't think it actually works. Surprisingly, when you try to read one file, it goes fine. But if you give a glob, fsspec does path translation of some sort, and always adds a disk name into the start, creating a pattern like:

(?s:[\\\\/][\\\\/]localhost[\\\\/][\\\\/]C\\$[\\\\/]git_reps[\\\\/]dlt[\\\\/]tests[\\\\/]common[\\\\/]storages[\\\\/]samples[\\\\/].*)\\Z

Here:
https://github.com/fsspec/filesystem_spec/blob/37c1bc63b9c5a5b2b9a0d5161e89b4233f888b29/fsspec/utils.py#L742

So, basically, it can see files by UNC path (I logged, it really does), but it filters them out, not returning anything, here:
https://github.com/fsspec/filesystem_spec/blob/37c1bc63b9c5a5b2b9a0d5161e89b4233f888b29/fsspec/spec.py#L608

However, if I give a file path (not a glob), it reads the file correctly, not going to this part of code, which does the path translation and pattern comparison.

If I give a normal full path, like C:/a/b/c, glob also works fine. But UNC - no.

IlyaFaer · 2024-04-16T09:50:57Z

@rudolfix, the only way I see how to fix it, is to write our own filesystem, inheriting the fsspec.LocalFileSystem, and override the glob method to fix this translation. As there are no conditions in the translation, there is no way to skip it for globs (it's skipped only for precise paths). What do you think?

rudolfix · 2024-04-16T09:54:40Z

@IlyaFaer

@rudolfix, the only way I see how to fix it, is to write our own filesystem, inheriting the fsspec.LocalFileSystem and override the glob method to fix this translation. As there are no conditions in the translation, there is no way to skip it for globs (it's skipped only for precise paths). What do you think?

heh this is so bad :) could you check first if regular Python glob works correctly? then my take would be to use it for file:// and just translate the results to something fsspec understand. but let's check it out.

IlyaFaer · 2024-04-16T09:57:17Z

@rudolfix, yes, Python works fine, I tried it even before started to look at fsspec

tests/load/filesystem/test_filesystem_common.py

rudolfix

there are things that still does not work

dlt/common/storages/fsspec_filesystem.py

rudolfix · 2024-04-17T23:41:05Z

dlt/common/storages/fsspec_filesystem.py

-        ).geturl()
+
+        if is_unc_path:
+            file_name = file


name must contain only name (final path component)

Well, not exactly. There is a relpath used right now:

dlt/dlt/common/storages/fsspec_filesystem.py

Line 305 in 77e2499

file_name = posixpath.relpath(file, bucket_url_no_schema)

So, it includes folder names too (if the bucket is samples, then it gives names like csv/freshman_kgs.csv):

dlt/tests/common/storages/utils.py

Lines 40 to 50 in 77e2499

"csv/freshman_kgs.csv",

"csv/freshman_lbs.csv",

"csv/mlb_players.csv",

"csv/mlb_teams_2012.csv",

"jsonl/mlb_players.jsonl",

"met_csv/A801/A881_20230920.csv",

"met_csv/A803/A803_20230919.csv",

"met_csv/A803/A803_20230920.csv",

"parquet/mlb_players.parquet",

"gzip/taxi.csv.gz",

"sample.txt",

Is it supposed to work like this? I didn't ask, because tests expect relative paths, not just names, but now when you said it, I start to suspect.

The problem is: for UNC relpath doesn't want to work. Not a problem to take a file name, but it'll be not the same as ordinary paths return. Relative paths for UNC seem problematic.

So, with UNC it'll be freshman_kgs.csv, but with ordinary path it's csv/freshman_kgs.csv. I suppose it'll be better if they return the same.

good point! although our docs mention that file_name can be a relative path, it is very misleading. so let's fix it:

rename file_name to relative_path and add file_name with just the name part

update all the tests

update documentation: https://dlthub.com/docs/dlt-ecosystem/verified-sources/filesystem#fileitem-representation

luckily we do not use file_name in verified sources

tests/common/storages/test_local_filesystem.py

…ath_fix

…o windows_path_fix

dlt/common/storages/fsspec_filesystem.py

IlyaFaer · 2024-04-18T08:59:08Z

dlt/common/storages/fsspec_filesystem.py

+
+        if is_unc_path:
+            file_url = "file:///" + file
+            file_name = file.replace(bucket_url_no_schema, "").replace("\\", "/").lstrip("/")


It's the most straightforward and stupid solution, but I think it'll work. For UNC we're getting file from glob, which is supposed to always return the same result, so str edits like this should work. This way it produces the same result as relpath for normal paths.

what is file here? file_url must contain UNC path so we are able to use it to open later. make sure it is like that

rudolfix · 2024-04-21T18:33:45Z

tests/common/storages/test_local_filesystem.py

+        assert len(all_file_items) == len(expected_files)
+
+        for file in all_file_items:
+            assert file["file_name"] in expected_files


@IlyaFaer what is the problem with using assert_sample_files(all_file_items, filesystem, config, load_content) here? this glob must work like any other globs. returned file details must be identical

this will also try to load the content of the file so it will verify if fsspec open works. I think it is crucial here

@rudolfix, opening doesn't work. Hm-m, I didn't notice it, but our code uses fsspec.open(), which (just like any other fsspec method) breaks UNC paths. O-o-okay, I added like a separate branch for UNC. Locally, the tests all passed, pushing (there are still several small things I didn't do, but I wanna see if I didn't break anything).

rudolfix · 2024-04-21T18:58:10Z

dlt/common/storages/fsspec_filesystem.py

@@ -276,19 +279,30 @@ def glob_files(
    """
    import os

+    is_unc_path = "$" in bucket_url


why do you assume that all paths with "$" are UNC? The precondition is that protocol must be "file://"
and there are better ways to recognize this: https://stackoverflow.com/questions/76209122/how-to-detect-a-windows-network-path

I don't think this splitdrive works so well:

Both in traditional and non-traditional cases, the first element is '', which means the drive wasn't split. The glob in the main time will handle all of them.

dlt/common/storages/fsspec_filesystem.py

rudolfix · 2024-04-21T19:03:57Z

dlt/common/storages/fsspec_filesystem.py

-        ).geturl()
+
+        if is_unc_path:
+            file_name = file


good point! although our docs mention that file_name can be a relative path, it is very misleading. so let's fix it:

rename file_name to relative_path and add file_name with just the name part

update all the tests

update documentation: https://dlthub.com/docs/dlt-ecosystem/verified-sources/filesystem#fileitem-representation

luckily we do not use file_name in verified sources

rudolfix · 2024-04-21T19:06:40Z

dlt/common/storages/fsspec_filesystem.py

+
+        if is_unc_path:
+            file_url = "file:///" + file
+            file_name = file.replace(bucket_url_no_schema, "").replace("\\", "/").lstrip("/")


what is file here? file_url must contain UNC path so we are able to use it to open later. make sure it is like that

…ve paths forms, improves tests

…veral globs to tests

IlyaFaer · 2024-04-29T10:44:51Z

@rudolfix, uf-f, that's quite a big re-writing. But I think I understood it

rudolfix · 2024-04-29T15:34:17Z

@IlyaFaer it was actually impossible to correctly implement all file paths styles on all platforms using fsspec. what this PR does is to use Python glob for all file:// uris. it also converts all file:// uris to native paths before opening. somehow fsspec is able to open UNC paths, but it is not able to do that when they are in file:// form.

as usual tests are way more complicated than code...

sultaniman · 2024-04-29T15:32:29Z

dlt/common/storages/configuration.py

+        local_path = unquote(uri.path)
+        if uri.netloc:
+            # or UNC file://localhost/path
+            local_path = "//" + unquote(uri.netloc) + local_path


is it intended to be // or there is one more /?

sultaniman · 2024-04-29T15:33:52Z

dlt/common/storages/configuration.py

            raise ConfigurationValueError(
-                "File path or netloc missing. Field bucket_url of FilesystemClientConfiguration"
-                " must contain valid url with a path or host:password component."
+                "File path and netloc are missing. Field bucket_url of"


I think File path and netloc are missing. will confuse people, to avoid this should we mention that we are using urlparse or URI?

sultaniman · 2024-04-29T15:40:39Z

dlt/common/storages/configuration.py

+        return str(pathlib.Path(local_path))
+
+    @staticmethod
+    def make_file_uri(local_path: str) -> str:


I think this could have been in some dlt.common.destination.utils|filesystem helpers

you are right that this needs a better place. we have file utils in FileStorage but they probably need to go somewhere else, then we'll move those as well

sultaniman · 2024-04-29T15:40:55Z

dlt/common/storages/configuration.py

+        return not uri_parsed.scheme or os.path.isabs(uri)
+
+    @staticmethod
+    def make_local_path(file_uri: str) -> str:


I think this could have been in some dlt.common.destination.utils|filesystem helpers

IlyaFaer added 2 commits April 10, 2024 11:31

fix(filesystem): UNC paths are not supported

8e111c1

small fix

45d1d7b

IlyaFaer mentioned this pull request Apr 11, 2024

test(filesystem): add a test for Windows UNC paths dlt-hub/verified-sources#423

Closed

IlyaFaer changed the title ~~Windows path fix~~ fix(filesystem): UNC paths are not supported Apr 11, 2024

IlyaFaer requested a review from rudolfix April 11, 2024 13:31

rudolfix reviewed Apr 11, 2024

View reviewed changes

rudolfix requested changes Apr 12, 2024

View reviewed changes

IlyaFaer added 2 commits April 17, 2024 12:06

use glob for UNC paths

e39b8bc

Merge branch 'devel' into windows_path_fix

de82bb5

IlyaFaer commented Apr 17, 2024

View reviewed changes

tests/load/filesystem/test_filesystem_common.py Outdated Show resolved Hide resolved

IlyaFaer added 3 commits April 17, 2024 14:23

add the third slash

7c465bc

fix slahes

6fee728

fix if

f43a13a

IlyaFaer marked this pull request as ready for review April 17, 2024 10:44

rudolfix added 3 commits April 17, 2024 22:47

Merge branch 'devel' into windows_path_fix

e813a3c

less invasive switch to Python glob

95edfaf

moves unc test to local so it runs on windows

50c6f2b

rudolfix requested changes Apr 17, 2024

View reviewed changes

IlyaFaer added 3 commits April 18, 2024 11:33

Merge branch 'devel' of https://github.com/dlt-hub/dlt into windows_p…

c045539

…ath_fix

Merge branch 'windows_path_fix' of https://github.com/dlt-hub/dlt int…

57a6b47

…o windows_path_fix

review fixes

2f1346c

IlyaFaer commented Apr 18, 2024

View reviewed changes

dlt/common/storages/fsspec_filesystem.py Outdated Show resolved Hide resolved

IlyaFaer commented Apr 18, 2024

View reviewed changes

rudolfix requested changes Apr 21, 2024

View reviewed changes

IlyaFaer and others added 14 commits April 22, 2024 16:41

test UNC path reading

0b8e04e

add docs

7c1bdfb

lint ignores

e7a893f

lint fix

fbcffbb

lint fix

f111714

excess ignore

7c0a918

lint fix

5232ce6

Merge branch 'devel' into windows_path_fix

98b985f

adds utils to convert from file to local path to filesystem config

e0ef3a9

offloads globbing to Python glob for locat paths, allows UNC and nati…

d983976

…ve paths forms, improves tests

allows the test file bucket to be relative path, fixes tests, adds se…

3810f80

…veral globs to tests

bumps dev adlfs version up to have working glob

44cca36

updates filesystem docs

23aee6d

fixes UNC to file uri

556a256

rudolfix approved these changes Apr 29, 2024

View reviewed changes

rudolfix merged commit 53dfed2 into devel Apr 29, 2024
50 checks passed

rudolfix deleted the windows_path_fix branch April 29, 2024 15:34

sultaniman reviewed Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(filesystem): UNC paths are not supported #1209

fix(filesystem): UNC paths are not supported #1209

IlyaFaer commented Apr 11, 2024

netlify bot commented Apr 11, 2024 •

edited

Loading

IlyaFaer commented Apr 11, 2024 •

edited

Loading

rudolfix left a comment

IlyaFaer commented Apr 12, 2024 •

edited

Loading

rudolfix left a comment

rudolfix left a comment

rudolfix Apr 12, 2024

IlyaFaer Apr 17, 2024 •

edited

Loading

IlyaFaer commented Apr 16, 2024 •

edited

Loading

IlyaFaer commented Apr 16, 2024 •

edited

Loading

rudolfix commented Apr 16, 2024

IlyaFaer commented Apr 16, 2024

rudolfix left a comment

rudolfix Apr 17, 2024

IlyaFaer Apr 18, 2024 •

edited

Loading

rudolfix Apr 21, 2024

IlyaFaer Apr 18, 2024

rudolfix Apr 21, 2024

rudolfix Apr 21, 2024

rudolfix Apr 21, 2024

IlyaFaer Apr 22, 2024 •

edited

Loading

rudolfix Apr 21, 2024

IlyaFaer Apr 22, 2024

rudolfix Apr 21, 2024

rudolfix Apr 21, 2024

IlyaFaer commented Apr 29, 2024

rudolfix commented Apr 29, 2024

sultaniman Apr 29, 2024

sultaniman Apr 29, 2024

sultaniman Apr 29, 2024

rudolfix Apr 29, 2024

sultaniman Apr 29, 2024

	"csv/freshman_kgs.csv",
	"csv/freshman_lbs.csv",
	"csv/mlb_players.csv",
	"csv/mlb_teams_2012.csv",
	"jsonl/mlb_players.jsonl",
	"met_csv/A801/A881_20230920.csv",
	"met_csv/A803/A803_20230919.csv",
	"met_csv/A803/A803_20230920.csv",
	"parquet/mlb_players.parquet",
	"gzip/taxi.csv.gz",
	"sample.txt",

fix(filesystem): UNC paths are not supported #1209

fix(filesystem): UNC paths are not supported #1209

Conversation

IlyaFaer commented Apr 11, 2024

netlify bot commented Apr 11, 2024 • edited Loading

✅ Deploy Preview for dlt-hub-docs canceled.

IlyaFaer commented Apr 11, 2024 • edited Loading

rudolfix left a comment

Choose a reason for hiding this comment

IlyaFaer commented Apr 12, 2024 • edited Loading

rudolfix left a comment

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaFaer Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

IlyaFaer commented Apr 16, 2024 • edited Loading

IlyaFaer commented Apr 16, 2024 • edited Loading

rudolfix commented Apr 16, 2024

IlyaFaer commented Apr 16, 2024

rudolfix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaFaer Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaFaer Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaFaer commented Apr 29, 2024

rudolfix commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Apr 11, 2024 •

edited

Loading

IlyaFaer commented Apr 11, 2024 •

edited

Loading

IlyaFaer commented Apr 12, 2024 •

edited

Loading

IlyaFaer Apr 17, 2024 •

edited

Loading

IlyaFaer commented Apr 16, 2024 •

edited

Loading

IlyaFaer commented Apr 16, 2024 •

edited

Loading

IlyaFaer Apr 18, 2024 •

edited

Loading

IlyaFaer Apr 22, 2024 •

edited

Loading