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.

Sign up for GitHub

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

Fix GCSFS walk() method #561

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a18399
Fix GCSFS walk() method
megaserg Jun 14, 2020
b87919b
[gcsfs] Consider trailing slash when skipping directories
megaserg Jun 23, 2020
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ def isdir(self, path):
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)
except OSError:
return False

Expand All @@ -41,26 +38,23 @@ def walk(self, path):
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']
if obj_path == path:
Copy link

Choose a reason for hiding this comment

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

This check still fails to catch the desired directory. obj_path has an extra '/' on the end which path does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was able to reproduce, but for some reason only when I provided a directory that didn't contain files, only other directories.

continue
if obj['type'] == 'directory':
directories.add(obj_path)
elif obj['type'] == 'file':
files.add(obj_path)

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