[DataPipe] Correct the type of exception that is being raised by ShufflerMapDataPipe

[NivekT]

Stack from ghstack:

• -> [DataPipe] Correct the type of exception that is being raised by ShufflerMapDataPipe #82666

Fixes pytorch/data#708

The following code snippet used to fail, now it has been added as a test case:

dp1 = dp.map.SequenceWrapper(range(10))
shuffle_dp1 = dp1.shuffle()
dp2 = dp.map.SequenceWrapper(range(10))
shuffle_dp2 = dp2.shuffle()
zip_dp = shuffle_dp1.zip(shuffle_dp2)
list(zip_dp)  # This used to fail

The issue was that ShufflerMapDataPipe raises a KeyError when an out of bound index is passed into it, but that was not handled by zip_dp's __getitem__ which only handled IndexError. With this change, it handles both.

[facebook-github-bot]

🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/82666
• 📄  Preview Python docs built from this PR
• 📄  Preview C++ docs built from this PR
• ❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (3 Pending)

As of commit 8d9ac2f (more details on the Dr. CI page):

Expand to see more

---

💚 💚 Looks good so far! There are no failures yet. 💚 💚

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[ejguan]

Then, why not change Shuffler to raise IndexError when key doesn't exist?

[NivekT]

Then, why not change Shuffler to raise IndexError when key doesn't exist?

I have considered that and I'm mostly indifferent. I found the definitions:

IndexError is raised when trying to access an invalid index within a list , the KeyError is raised when accessing an invalid key within a dict.

The exception comes from Shuffler's __getitem__, it is a KeyError because it is raised by a dict. This makes sense to me since I don't necessarily think of MapDataPipe as a list.

More practically, I worry that if there is another MapDataPipe that raises KeyError rather than IndexError for a similar reason, Zipper will not be able to handle it. Therefore fixing it for Zipper may make more sense.

[ejguan]

The exception comes from Shuffler's __getitem__, it is a KeyError because it is raised by a dict. This makes sense to me since I don't necessarily think of MapDataPipe as a list.

Even though we don't necessarily think of MapDataPipe as a list, users still want to do list(dp) right? This is the reason that IndexError is still well handled by list inside Zipper. Let's say I have a custom DataPipe:

class DP(MapDataPipe):
    def __getitem__(self, index):
        return self.dp[index]
    def __len__(self):
        return len(self.dp)

dp = SequenceWrapper(...).shuffle()
dp = DP(dp)
list(dp)

I would expect KeyError is raised as well. IndexError is used as a protocol for list function to notice the end of iteration. Without changing Shuffler, we are adding extra requirement for users to implement all DataPipe handling KeyError in __getitem__.

More practically, I worry that if there is another MapDataPipe that raises KeyError rather than IndexError for a similar reason, Zipper will not be able to handle it. Therefore fixing it for Zipper may make more sense.

I think this would be more dangerous as such KeyError might be expected. We are silently suppressing such Error.

WDYT?

[NivekT]

If we leave Zipper as it is, aren't we expecting dp[index] to always raise IndexError if the index is out of bound (for any dp that gets passed into Zipper)?

This is because we aren't calling list() inside Zipper, but dp[index]

[ejguan]

If we leave Zipper as it is, aren't we expecting dp[index] to always raise IndexError if the index is out of bound (for any dp that gets passed into Zipper)?

Yeah. Here is the link from Python official about __getitem__: https://docs.python.org/3/reference/datamodel.html#object.__getitem__
And, here is the link for the definition of Sequence and Mapping: https://docs.python.org/3/library/collections.abc.html#collections-abstract-base-classes

I would expect MapDataPipe as a Sequence since only __getitem__ and __len__ are required.

[NivekT]

Okay if we treat MapDataPipe as Sequence then it makes sense to always raise IndexError. I can make the change.

[ejguan]

LGTM, thanks

[NivekT]

@pytorchbot merge

[pytorchmergebot]

@pytorchbot successfully started a merge job. Check the current status here