Waiting for lint to pass

We specifically moved that line out of the if condition in a previous pr.

This is not correct and makes rng generation depending on num_workers. I don't know why you would assume rng to be unchanged for different code. Finally, this seed has nothing to do with shuffle.

I don't follow. The random vector appears to be unused when num_workers == 0

I guess the in question is PR #7265

cc @vfdev-5 @apaszke

The aim of #7265 was two-fold 1) to have a reproducible behaviour of batch sampler (when shuffle=True) for any number of workers and 2) have a possibility to skip previous batches by simply running multiple times next on the batch sampler (context: resuming crashed training and DataLoader state from a given iteration).

I agree with the remark that rng state should not be changed while running dataloader with shuffle=False. @InnovArul however proposed fix will still change rng if num_worker > 0 in the test dataloader, non ?

@vfdev-5 you are right! the fix would not work if num_workers > 0.
What do you think of this suggestion?

• initialize base_seed in the constructor of Dataloader itself
• access it from _DataLoaderIter (loader.base_seed)

I am not sure if it affects the old fixes.

@InnovArul maybe there is another way (without introducing a supplementary variable) to check this by testing isinstance(loader.sampler, SequentialSampler).
I thought also maybe it would be helpful later to introduce the test of all those behaviors:
a) check indices when shuffle=True in two cases: num_workers=0 and num_workers > 0 (example)
b) check rng unchanged after test loader execution.

@ezyang @ssnl what do you think about all of this ?

• Yes, the requirement is to not to update rng state if shuffle=False regardless of num_workers.
  Testing the sampler variable seems to be a good fix as well.

• Also, if we introduce a supplementary variable, the 2nd aim of Fix issue #7209 in DataLoader #7265 (have a possibility to skip previous batches) is getting jeopardized. So, introducing a new variable may not be a good option.

Yes, the requirement is to not to update rng state if shuffle=False regardless of num_workers.
Testing the sampler variable seems to be a good fix as well.

I am really not sure about this. The seed generated here is completely unrelated to shuffle, and is necessary for multiprocessing loading, even if you use sequential sampler.

I understand. shuffle has nothing to do with base_seed.

The use-case is as follows (from the pytorch forum thread):

• Set manual seed for torch, numpy etc.
• usually, we initialize dataloaders with: shuffle=True for train data loader, shuffle=False for validation and test data loaders.
• then call methods in a loop : 1) train(train_dataloader), 2) validation(val_dataloader), 3) test(test_dataloader) - optional.
• we expect train_dataloader to produce same set of indices regardless of the validation(), test() function calls. (i.e., if only validation() is called (or) if both validation(), test() are called)

With the current implementation, we are not achieving this behavior, as the rng state is modified by val_dataloader, test_dataloader by default.

The seed generated here is completely unrelated to shuffle, and is necessary for multiprocessing loading, even if you use sequential sampler.

Oh, yes, that's true, I forgot about that:

@ssnl does it make sense to make :

random.seed(seed) 
torch.manual_seed(seed)

optional in case of shuffle=False ?

we expect train_dataloader to produce same set of indices regardless of whether we call validation(), test() functions.

@InnovArul I would say that the requirement is only that train_loader produces same indices when prefixed with torch.manual_seed(seed)...

@vfdev-5 thats right! I forgot to mention the setting of manual seed.

Just checking with you guys to update/close the request before we forget this.
Do I need to change anything with the code? / Do you think this change is unnecessary or so?

OK, I finally read through all of the background information. At the end of day, the root question is whether or not you expect Dataloader (without shuffle) to consume a random number. It seems that the expectation of a lot of people in this thread is that it should not. But we have to compute a base seed for the workers (to ensure that they are deterministic), and it should be randomized every time you call Dataloader, so it does seem like the Dataloader rng call is merited. So I agree with @ssnl that we shouldn't move the base seed in the conditional; better for us to just unconditionally consume the seed, as it makes the contract regarding how many RNG numbers the function consumes more clear. So we will not accept this PR.

However, I think there are some useful follow PRs that we could accept.

• We could add documentation to Dataloader making it more clear that it consumes RNG, even when shuffle is not set.
• We could add a new seed keyword argument to the constructor, letting you explicitly specify the starting seed for the subjobs (letting you skip the RNG call)

So if you want to make a PR for any of these things, CC me and I'll review and land them.

For the particular user problem here, if he really liked his results from pipeline one, I'd suggest resetting the rng as suggested in https://discuss.pytorch.org/t/dataloader-problem-problem-arises-when-shuffle-true/45631/24 is a good workaround (if the seed keyword was added, this wouldn't be necessary; just pass a hardcoded seed for your workers). Alternately, you could unconditionally construct the Dataloader object to ensure the RNG is consumed in the same way in both cases.

@ezyang Thanks for the detailed reply!
I will do the documentation and adding base_seed parameter to the constructor.

Before that, I would like to get a clarification,

we have to compute a base seed for the workers (to ensure that they are deterministic), and it should be randomized every time you call Dataloader

Do you mean, a separate seed has to be computed every time when the Dataloader __iter__ is called?
(or) Do you mean that the seed has to be computed only once per Dataloader?

I understand from your pointer to add base_seed argument to the Dataloaders contructor that the seed is fixed per Dataloader instance. If so, can we move the base_seed determination (unless base_seed is passed by the user) to the Dataloader constructor(__init__) and clearly document that Dataloader construction consumes one RNG state? (as mentioned here). Does it make sense?

I understand from your pointer to add base_seed argument to the Dataloaders contructor that the seed is fixed per Dataloader instance. If so, can we move the base_seed determination (unless base_seed is passed by the user) to the Dataloader constructor(init) and clearly document that Dataloader construction consumes one RNG state? (as mentioned here). Does it make sense?

This definitely can not happen, as it would make every epoch generate the exact same random augmentations. However, I think the better way is to accept a torch.Generator object (that is defaulted as torch.default_generator).

But even then I have some concerns that the added argument would be confusing, because it only controls the randomness for that particular seed, and does not affect Samplers. Our Samplers currently do not take in custom generators (neither CPU nor CUDA).

ok. Thanks both. It seems RNG consumption is unavoidable even during Shuffle=False due to the need for random transformations. I will try to add it in the documentation.

We already have a mention about base_seed in the Dataloader's documentation. I will add a line nearby that.

By default, each worker will have its PyTorch seed set to base_seed + worker_id, where base_seed is a long generated by main process using its RNG.

I rebased my repo to latest pytorch to update this pull request. But it has messed up the history.
I will create another pull request for it. Hence, closing this pull request. Sorry.