Set reshuffle_each_iteration in Dataset.shuffle() directly to True to avoid confusion

[DanielYang59]

Summary

Set the default reshuffle_each_iteration value in tf.data.Dataset.shuffle method to True directly (previous was None but was interpreted to True) to raise awareness of possible silent data leakage, see discussion #59279.

Details (copied and cleaned up from #59279)

The Dataset.shuffle method might lead to dangerously silent data leakage:

In the Dataset doc under the shuffle method, it reads:

shuffle(
buffer_size, seed=None, reshuffle_each_iteration=None, name=None
)

where the reshuffle_each_iteration=None is super misleading, as it be easily misinterpreted that reshuffled is off by default (which is not true at all).

The shuffle method, which called shuffle_op._shuffle:

tensorflow/tensorflow/python/data/ops/dataset_ops.py

Lines 1472 to 1473 in 4dacf3f

	return shuffle_op._shuffle( # pylint: disable=protected-access
	self, buffer_size, seed, reshuffle_each_iteration, name=name)

which then called _ShuffleDataset:

tensorflow/tensorflow/python/data/ops/shuffle_op.py

Lines 25 to 32 in b756c44

	def _shuffle( # pylint: disable=unused-private-name
	input_dataset,
	buffer_size,
	seed=None,
	reshuffle_each_iteration=None,
	name=None):
	return _ShuffleDataset(
	input_dataset, buffer_size, seed, reshuffle_each_iteration, name=name)

which finally inititate the _ShuffleDataset class:

tensorflow/tensorflow/python/data/ops/shuffle_op.py

Lines 35 to 50 in b756c44

	class _ShuffleDataset(dataset_ops.UnaryUnchangedStructureDataset):
	"""A `Dataset` that randomly shuffles the elements of its input."""
	def __init__(self,
	input_dataset,
	buffer_size,
	seed=None,
	reshuffle_each_iteration=None,
	name=None):
	"""See `Dataset.shuffle()` for details."""
	self._input_dataset = input_dataset
	self._buffer_size = ops.convert_to_tensor(
	buffer_size, dtype=dtypes.int64, name="buffer_size")
	self._seed, self._seed2 = random_seed.get_seed(seed)
	if reshuffle_each_iteration is None:
	reshuffle_each_iteration = True

has the following dangerous definition:

if reshuffle_each_iteration is None:
      reshuffle_each_iteration = True

As a result, the default reshuffle_each_iteration = None would be interpreted to reshuffle_each_iteration = True (which is truly unexpected by user).

TODO list:

• Set the default value of reshuffle_each_iteration directly to True
• Add warning in docs about possible data leakage related to reshuffle_each_iteration = True
  Issue warning when training/validation datasets are split by using the shuffle + take/skip pattern (scheduled for a separate follow-up PR)

[DanielYang59]

I think I might keep the 3rd task to a separate PR, as I'm still working on a proper way to track if dataset has undergone shuffle(reshuffle_each_iteration=True) before take and skip. And this PR would not change any behaviour as far as I know. Can you please review? @aaudiber @gbaned

[aaudiber]

Thank you!

[aaudiber]

Test is failing because we need to update the golden files with the API change:

tensorflow/tensorflow/tools/api/golden/v1/tensorflow.data.-dataset.pbtxt

Lines 190 to 193 in fb15a8c

	member_method {
	name: "shuffle"
	argspec: "args=[\'self\', \'buffer_size\', \'seed\', \'reshuffle_each_iteration\', \'name\'], varargs=None, keywords=None, defaults=[\'None\', \'None\', \'None\'], "
	}

tensorflow/tensorflow/tools/api/golden/v2/tensorflow.data.-dataset.pbtxt

Lines 157 to 160 in fb15a8c

	member_method {
	name: "shuffle"
	argspec: "args=[\'self\', \'buffer_size\', \'seed\', \'reshuffle_each_iteration\', \'name\'], varargs=None, keywords=None, defaults=[\'None\', \'None\', \'None\'], "
	}

Please updates the Nones to True there as well

[DanielYang59]

Thanks for reviewing and the comments @aaudiber . I'm not quite aware of the API and thanks for the input!

[DanielYang59]

Hi @aaudiber. Thanks for reviewing. Also I noticed some tests are failing (ROCm/MacOS CPU and such) though they don't have a Required tag. Should I be concerned?

[DanielYang59]

This PR has been approved and taged ready to pull for over a month now. Is there anything I should do? Thanks! @gbaned @aaudiber