I wonder if we should instead change the MutableSequence check to Sequence? That would allow that path to automatically work for memoryview, and would avoid spurious warnings that precede errors when readonly sequences like tuple.

There's certainly some contention that me doing this downstream was sub-optimal anyway, but I don't think there's any claim that it should be disallowed/considered a bug to shuffle a memoryview.

That was specific to that particular use of memoryviews post-shuffle, not the shuffling per se. This is a good change.

Both changes seem fine to me (isinstance check or Sequence). Since a non-mutable sequence should error already, I somewhat doubt that modifying that as Eric suggested has any downsides. Presumably, any of the tensor frameworks will either be both or neither. So I think Eric's suggestion is probably good.

I am not quite sure that the memoryview change can't go wrong in principle (memoryviews do support suboffsets for example), but I am willing to take the risk of such an unlikely backcompat break (it would go to an error), it probably is nicer for memoryviews. Although, I doubt there is a reason to optimize for them here.

I pushed in Eric's suggestion after confirming the random suite still passes locally for this feature branch.

Thanks, LGTM. I find it a bit surprising that a memoryview isn't considered a mutable sequence (is that supposed to be only the length?), but I guess if Python changes it, we have a test that notifies us.

I would be fine with a memoryview fast path as you had it, although I guess this might be nicer for backporting.

MutableSequences have to implement insert, which memoryview would never do.

Ah right. So I think this is still good enough to warn for tensorflow as well? In which case I think we should go with it or is there some other reason to prefer MutableSequence, that I am missing?

Although, I would be happy to entertain the thought of making things more strict in 1.21.

Thanks Tyler.