Solve issue #282: Update FSSpecFileLister, IoPathFileLister #383

ykabusalah · 2022-05-04T18:53:16Z

Fixes #282

Summary

Updated FSSpecFileLister and IoPathFileLister to optionally take a list of strings as root paths. As per the contributors guideline, added tests to the testsuite to cover changes and ran the pre-commit format checker.

Changes

Updated FSSpecFileLister to take either a str or sequence of strings as root path argument (in _init_ arguments)
Updated _init_ of FSSpecFileLister to check if root argument is string or list and make into IterDataPipe
Updated _iter_ of FSSPecFileLister to look through all root paths when yielding files
Added unit test in test_fsspec.py to test for multiple root path input to FSSpecFileLister
Updated IoPathFileLister to take either a str or sequence of strings as root path argument (in _init_ arguments)
Updated _init_ of IoPathFileLister to check if root argument is string or list and make into IterDataPipe
Updated _iter_ of IoPathFileLister to look through all root paths when yielding files
Added unit test in test_local_io.py to test for multiple root path input to IoPathFileLister

facebook-github-bot · 2022-05-04T18:53:20Z

Hi @ykabusalah!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2022-05-04T19:49:28Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

ykabusalah · 2022-05-04T21:58:56Z

@NivekT one test failed for macos build but I see that it has failed on previous pull requests as well. Let me know if there is anything I need to do for this.

Thanks.

NivekT · 2022-05-04T22:03:36Z

@NivekT one test failed for macos build but I see that it has failed on previous pull requests as well. Let me know if there is anything I need to do for this.

Thanks.

Don't worry about that one. I will have a look at the rest of your PR soon. Thanks!

ykabusalah · 2022-05-04T22:04:46Z

@NivekT one test failed for macos build but I see that it has failed on previous pull requests as well. Let me know if there is anything I need to do for this.
Thanks.

Don't worry about that one. I will have a look at the rest of your PR soon. Thanks!

Awesome. Sounds good. Any chance you'd be able to take a look at it by tomorrow?

facebook-github-bot · 2022-05-05T00:23:30Z

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ejguan

Thank you for adding this patch.

ejguan · 2022-05-05T18:17:24Z

torchdata/datapipes/iter/load/fsspec.py

-                        yield fs.protocol + "://" + abs_path
+        for root in self.datapipe:
+            fs, path = fsspec.core.url_to_fs(root)
+            is_local = fs.protocol == "file" or not root.startswith(fs.protocol)


Do you mind solving #348 as well?
You can basically check if fs.protocol is a list. If it is, we can run any(root.startswith(protocol) for protocol in fs.protocol)

ejguan · 2022-05-05T18:17:31Z

torchdata/datapipes/iter/load/fsspec.py

+                        continue
+
+                    # ensure the file name has the full fsspec protocol path
+                    if file_name.startswith(fs.protocol):


ejguan · 2022-05-05T18:18:13Z

torchdata/datapipes/iter/load/fsspec.py

+                        if root.startswith(fs.protocol):
+                            yield fs.protocol + "://" + abs_path


The logic here might need to be changed as well.

In what way should this be changed?

I believe @ejguan is referring to his other comment here:
#383 (comment)

It will be great if you can solve #348 as well in this PR, and when you do, the logic here will have to change as well.

Suggested change

if root.startswith(fs.protocol):

yield fs.protocol + "://" + abs_path

if isinstance(fs.protocol, list):

for protocol in fs.protocol:

if root.startswith(protocol):

yield protocol + "://" + abs_path

break

elif root.startswith(fs.protocol):

yield fs.protocol + "://" + abs_path

test/test_fsspec.py

NivekT · 2022-05-05T20:35:53Z

test/test_local_io.py

+    def test_io_path_file_lister_iterdatapipe_with_list(self):
+        datapipe = IoPathFileLister(root=[self.temp_sub_dir.name, self.temp_sub_dir_2.name])
+
+        # check all file paths within sub_folder are listed


nit: similar to my other comment about comparing sorted list instead

NivekT

Thanks for submitting the PR! Just a few small comments.

ykabusalah · 2022-05-05T23:10:26Z

@NivekT @ejguan

This fixes issue #348 and updates Testing Logic

Summary

Updated FSSpecFileLister to check if protocol for root is a string or list. S3 returns a list, so this handles that return and updates logic.
Updated IO and FSSpec unit tests to sort and compare lists rather than looping and checking

Changes

Updated FSSpecFileLister to check if fs protocol is a list or a string
If it is a string, it puts it into a list and handles the rest of the logic of the FileLister as if it were a list
Uses Python's "any()" functionality to check conditions against the protocol list
Check the beginning of the root against all protocols and returns if it is a match, else returns the absolute path
Updated IO and FSSpec unit tests to sort and compare lists rather than looping and checking

facebook-github-bot · 2022-05-05T23:18:18Z

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ejguan

Look good to me in general. Two more comments below.

ejguan · 2022-05-06T12:51:53Z

torchdata/datapipes/iter/load/fsspec.py

+            else:
+                protocol_list = fs.protocol
+
+            is_local = fs.protocol == "file" or not any(root.startswith(protocol) for protocol in fs.protocol)


you mean protocol_list, right?

torchdata/datapipes/iter/load/fsspec.py

… in a for loop to stop yielding values when one we wanted is found

ykabusalah · 2022-05-06T16:17:06Z

@ejguan @NivekT

This new commit should address the remaining outstanding concerns: added a variable change to make it congruent with the rest of the changes as well as a break in a for loop to stop yielding values when we found the one we wanted.

Thanks.

facebook-github-bot · 2022-05-06T17:04:56Z

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

NivekT

LGTM! Thanks for working on this PR!

ejguan

Thank you for fixing these issues! LGTM with two nit comments below.

torchdata/datapipes/iter/load/fsspec.py

torchdata/datapipes/iter/load/iopath.py

facebook-github-bot · 2022-05-18T21:59:10Z

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-05-18T21:59:38Z

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

this solves issue pytorch#282

a97ae10

update files to pass mypy tests locally

6217227

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 4, 2022

ejguan reviewed May 5, 2022

View reviewed changes

NivekT reviewed May 5, 2022

View reviewed changes

test/test_fsspec.py Show resolved Hide resolved

NivekT reviewed May 5, 2022

View reviewed changes

ykabusalah added 2 commits May 5, 2022 17:48

update unit tests to create lists of the File Lister Iterator

0a23d03

fix issue pytorch#348

490d9e6

ejguan reviewed May 6, 2022

View reviewed changes

added variable change to make congruent with rest of change and break…

7073313

… in a for loop to stop yielding values when one we wanted is found

NivekT approved these changes May 6, 2022

View reviewed changes

ejguan mentioned this pull request May 6, 2022

Add functional API to FileLister #387

Closed

ejguan approved these changes May 9, 2022

View reviewed changes

torchdata/datapipes/iter/load/fsspec.py Show resolved Hide resolved

torchdata/datapipes/iter/load/iopath.py Show resolved Hide resolved

facebook-github-bot closed this in a92f573 May 19, 2022

NivekT mentioned this pull request May 19, 2022

FSSpecFileLister error with S3 root #348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solve issue #282: Update FSSpecFileLister, IoPathFileLister #383

Solve issue #282: Update FSSpecFileLister, IoPathFileLister #383

ykabusalah commented May 4, 2022

facebook-github-bot commented May 4, 2022

facebook-github-bot commented May 4, 2022

ykabusalah commented May 4, 2022

NivekT commented May 4, 2022

ykabusalah commented May 4, 2022

facebook-github-bot commented May 5, 2022

ejguan left a comment

ejguan May 5, 2022

ejguan May 5, 2022

ejguan May 5, 2022

ykabusalah May 5, 2022

NivekT May 5, 2022

ejguan May 5, 2022

NivekT May 5, 2022

NivekT left a comment

ykabusalah commented May 5, 2022

facebook-github-bot commented May 5, 2022

ejguan left a comment

ejguan May 6, 2022

ykabusalah commented May 6, 2022

facebook-github-bot commented May 6, 2022

NivekT left a comment •

edited

Loading

ejguan left a comment

facebook-github-bot commented May 18, 2022

facebook-github-bot commented May 18, 2022

		if root.startswith(fs.protocol):
		yield fs.protocol + "://" + abs_path

-                        if root.startswith(fs.protocol):
-                            yield fs.protocol + "://" + abs_path
+                        if isinstance(fs.protocol, list):
+                            for protocol in fs.protocol:
+                                if root.startswith(protocol):
+                                   yield protocol + "://" + abs_path
+                                   break
+                        elif root.startswith(fs.protocol):
+                            yield fs.protocol + "://" + abs_path

Solve issue #282: Update FSSpecFileLister, IoPathFileLister #383

Solve issue #282: Update FSSpecFileLister, IoPathFileLister #383

Conversation

ykabusalah commented May 4, 2022

Summary

Changes

facebook-github-bot commented May 4, 2022

Action Required

Process

facebook-github-bot commented May 4, 2022

ykabusalah commented May 4, 2022

NivekT commented May 4, 2022

ykabusalah commented May 4, 2022

facebook-github-bot commented May 5, 2022

ejguan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NivekT left a comment

Choose a reason for hiding this comment

ykabusalah commented May 5, 2022

Summary

Changes

facebook-github-bot commented May 5, 2022

ejguan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykabusalah commented May 6, 2022

facebook-github-bot commented May 6, 2022

NivekT left a comment • edited Loading

Choose a reason for hiding this comment

ejguan left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 18, 2022

facebook-github-bot commented May 18, 2022

NivekT left a comment •

edited

Loading