-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Option to provide seed to random generators to ensure reproducibility #1572
Option to provide seed to random generators to ensure reproducibility #1572
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1572 +/- ##
======================================
Coverage 88% 88%
======================================
Files 70 70
Lines 4346 4346
======================================
Hits 3828 3828
Misses 518 518 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great feature! might i suggest a reorg moving this to pytorch_lightning/seed.py
? i think we are eventually trying to get rid of the utilities module in favor of more direct names.
Nice! Would it maybe be good to document for which libraries the seed is set (numpy, torch, random, ...)? This could maybe go to |
@kumuji mind check all examples (probably also elsewhere) and make then use its seeding |
@jamesjjcondon do you have any suggestions? 🐰 me personally not much fun of having packages with just one module (and fund inside) |
Also, I am sorry, I rebased manually on master, but seems like it was not a good idea since there is autorebase in github actions. Should I fix it? |
For seeding to work well with ddp the order needs to be: seed_everything()
# seed should be the first thing done because in ddp, model init needs to \\
# be the same on every process
model()
fit() |
In the last commit fixed it in the benchmark code - model was initialized before trainer, where seeding happens |
it is completely fine, in fact the rebase from action is nothing now.. and also cannot resolve conflicts... |
@kumuji We can't really see your changes here on Github, could you rebase onto master, please? |
48a2234
to
8ef0e05
Compare
On some seeds seems like lbfgs doesn't converge. So I fixed the seed during testing.
From Example to testcode Co-authored-by: Jirka Borovec <[email protected]>
It is failing on some versions, not very predictable
Co-authored-by: Jirka Borovec <[email protected]>
36b0fc0
to
2c97d5b
Compare
@williamFalcon done ^^ |
@kumuji amazing! |
I added small function in utilities which imports torch, numpy, python
random and sets seed for all of the libraries to ensure reproducibility
of results.
Before submitting
What does this PR do?
Partially fixes #1565 (issue).
This code makes sure to fix seed for all pseudo-random generators, to ensure the same results for different runs. But probably doesn't work fully with
ddp
.PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃