Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

set_seed in train_model #4662

Merged
merged 10 commits into from
Aug 1, 2022
Merged

set_seed in train_model #4662

merged 10 commits into from
Aug 1, 2022

Conversation

prajjwal1
Copy link
Contributor

@prajjwal1 prajjwal1 commented Jul 12, 2022

Patch description
Adding a function that allows user to set seed for training. User just has to pass the flag --set_seed with existing arguments to train_model.py in case they want to use a seed other than 42.

Testing steps

Other information

@prajjwal1
Copy link
Contributor Author

@stephenroller Can you please look into this ? It has been here for sometime now.

@prajjwal1
Copy link
Contributor Author

I was thinking of using the functionality brought from this to teachers also. Teacher files have SEED manually hardcoded to 42. How is a user supposed to set the seed in Teacher. This will allow user to bring this functionality to control rng in teacher.

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this, but I'm pretty sure the existing code already sets the seed. Can we double check?

@prajjwal1
Copy link
Contributor Author

I did check, i didn't find any way to setting the seed universally. I plan to send one more PR for multiwoz_v22, which will set the rng through opt. Current opt dict doesn't have it, so where else is seed parameter supposed to go ?

@klshuster
Copy link
Contributor

I'm fine with this, but I'm pretty sure the existing code already sets the seed. Can we double check?

interestingly I can't seem to find it either. we set seeds in nearly every other script except for train model

pure non-determinism! In that case, can we have the default seed be None and only set seed if it's not None?

@prajjwal1
Copy link
Contributor Author

Default way is to set the seed to 42 right ? Can I set it to 42, and this can be overridden by the user by passing in --seed parameter ? By not setting the seed, users are likely to observe variability in their training runs.

@stephenroller
Copy link
Contributor

I'm fine with this, but I'm pretty sure the existing code already sets the seed. Can we double check?

interestingly I can't seem to find it either. we set seeds in nearly every other script except for train model

pure non-determinism! In that case, can we have the default seed be None and only set seed if it's not None?

ah right this is what i misremembered

@stephenroller
Copy link
Contributor

Default way is to set the seed to 42 right ? Can I set it to 42, and this can be overridden by the user by passing in --seed parameter ? By not setting the seed, users are likely to observe variability in their training runs.

historically we've preferred backwards compatibility

@klshuster
Copy link
Contributor

I agree, let's keep it to None. We've advocated for variability from the start

@prajjwal1
Copy link
Contributor Author

@klshuster Is it fine now ?

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, will leave Kurt for final approval and merge

@prajjwal1 prajjwal1 requested a review from klshuster July 29, 2022 21:56
Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@klshuster klshuster merged commit 62e68f9 into facebookresearch:main Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants