add manual seeding for RL experiments

[natolambert]

@lvwerra or @younesbelkada is there any case where a model will be initialized randomly? If so, we need to make sure the seeds are set too.

Also, I ran make style and it modified those two lines of setup, so we can change them here or elsewhere.

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[younesbelkada]

Thanks a lot for this PR and taking care of the randomness!
I am very much ok for these changes, but I have few questions
1- Does this ensure exact reproducibility? I think you still need to set manual seed before creating each model. Maybe we should document more about this in the documentation
2- Maybe we should set the seed only if the seed is not None , but no strong opinion here maybe let's see what @lvwerra says here
Thanks again!

[natolambert]

I think we try it and see. It covers most use cases, I just don't really know how model loading etc is handled in transformers. My intuition is they're all pretrained and using torch, so the torch setting should work. Not so hard to check 🤗 (I can double check that)

[lvwerra]

As @younesbelkada pointed out we initialize the models before setting up the PPOTrainer. I think we could copy the set_seed function from transformers (we don't need the TF case) which also takes care of the CUDA case. Then we can use it in the PPOTrainer and also expose it to the user.

[natolambert]

What is the way the model is initialized? I'm confused because I thought we were fine-tuning existing models. What parts get initialized?

@lvwerra @younesbelkada -- seems like a gap in my new NLP knowledge :)

[lvwerra]

@natolambert it's actually the RL part: the value head is randomly initialized :)

[younesbelkada]

As a side note, from what I have discovered, you need to declare the seed before initializing any new module, i.e. in this case before initializing model & ref_model. Check the example snippet below:

import torch
import torch.nn as nn

torch.manual_seed(0)
linear_1 = nn.Linear(10, 10)
linear_2 = nn.Linear(10, 10)

# check weights are not the same
assert not torch.allclose(linear_1.weight, linear_2.weight)

torch.manual_seed(0)
linear_1 = nn.Linear(10, 10)
torch.manual_seed(0)
linear_2 = nn.Linear(10, 10)

# check weights are the same
assert torch.allclose(linear_1.weight, linear_2.weight)

If we make sure users do this, maybe it will be possible to ensure reproducibility - thus worth documenting it!

[natolambert]

I don't think we want all the weights to be the same, no? The initial seed should be enough to maintain reproducibility across runs -- does that make sense?

[natolambert]

If we are encountering issues later in this, we could also incorportate set_full_determinism

[younesbelkada]

(optional) this is a personal taste though, I feel it's better to not force users to set a seed by default

Suggested change

	random.seed(seed)
	# np.random.seed(seed) # if np is added to functionality, add this line
	torch.manual_seed(seed)
	torch.cuda.manual_seed_all(seed)
	if seed is not None and isinstance(seed, int):
	random.seed(seed)
	# np.random.seed(seed) # if np is added to functionality, add this line
	torch.manual_seed(seed)
	torch.cuda.manual_seed_all(seed)

[younesbelkada]

Thanks for your work on this @natolambert
I left few minor comments, IMO we should not force users to set a seed by default but maybe @lvwerra has a different opinion on this, or if you think it's ok to force users to set a seed by default then it's fine for me!

[natolambert]

I personally just don't really see an issue with the default seed being 0? It's very normal practice in RL imo. The users for non-research generally will just not touch it? Maybe you didn't see the default seed in the constructor, which would explain some of the confusion.

I don't know how a default seed is particularly different than no seeding.

[younesbelkada]

LGTM thanks for this! 🚀
Yes your explanation makes sense!

[lvwerra]

Looks good to me, just two minor comments. I think without setting the seed before initializing the value head (i.e. loading the models which is outside the trainer) the training will behave different no matter the internal seeding. That's why I would expose the set_seed as a standalone function that's used both internally and externally.

For the default value, the transformers Trainer uses 42 :)

[lvwerra]

lets maybe add it - don't know if torch or some other library is using numpy somewhere.

[lvwerra]

I would move the function to core.py and expose it to the user via __init__.py. So if we wanted we can use it in the script too.