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

Fix GCSFS walk() method #561

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions petastorm/gcsfs_helpers/gcsfs_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
path = norm_path(_stringify_path(path))
try:
contents = self.fs.ls(path)
if len(contents) == 1 and contents[0] == path:
return False
else:
return True
return not(len(contents) == 1 and contents[0] == path)

Check warning on line 15 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L15

Added line #L15 was not covered by tests
except OSError:
return False

Expand All @@ -41,26 +38,23 @@
directories = set()
files = set()

for key in self.fs.ls(path, detail=True):
for obj in self.fs.ls(path, detail=True):
# each info name must be at least [path]/part , but here
# we check also for names like [path]/part/
path = key['name']
if key['storageClass'] == 'DIRECTORY':
if path.endswith('/'):
directories.add(path[:-1])
else:
directories.add(path)
elif key['storageClass'] == 'BUCKET':
pass
else:
files.add(path)

files = sorted([posixpath.split(f)[1] for f in files
if f not in directories])
directories = sorted([posixpath.split(x)[1]
for x in directories])

yield path, directories, files
obj_path = obj['name']

Check warning on line 44 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L44

Added line #L44 was not covered by tests
if obj_path.strip('/') == path.strip('/'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use more conservative rstrip.

continue

Check warning on line 46 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L46

Added line #L46 was not covered by tests
if obj['type'] == 'directory':
directories.add(obj_path)

Check warning on line 48 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L48

Added line #L48 was not covered by tests
elif obj['type'] == 'file':
files.add(obj_path)

Check warning on line 50 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L50

Added line #L50 was not covered by tests

rel_files = sorted([posixpath.split(f)[1] for f in files
if f not in directories])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of checking if f not in directories? Could there be an entry in files that is also in directories?

rel_directories = sorted([posixpath.split(x[:-1])[1]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More self explanatory, IMO: x[:-1] -> x.rstrip('/').

for x in directories])

yield path, rel_directories, rel_files

Check warning on line 57 in petastorm/gcsfs_helpers/gcsfs_wrapper.py

View check run for this annotation

Codecov / codecov/patch

petastorm/gcsfs_helpers/gcsfs_wrapper.py#L57

Added line #L57 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When path is invalid, the function still yields once. This does not match os.walk semantics which would not yield at all in that case.


for directory in directories:
for tup in self.walk(directory):
Expand Down