[BUG] Check environ before selecting a seed to prevent warning message #4743

SeanNaren · 2020-11-18T16:21:54Z

What does this PR do?

Fixes #4741
Make sure we check the environ first before generating a seed to prevent warning message.
Check issue for reprod.

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

…necessary warning message

SeanNaren · 2020-11-18T16:36:02Z

Still fixing!

carmocca · 2020-11-18T16:37:33Z

You could add a pytest.warns test using the reproduction snippet given in the issue

SeanNaren · 2020-11-18T16:40:45Z

Thanks @carmocca! Will do :)

The fix is still WIP (just wanted to test if it worked). Ignores the seed set from cmdline atm. I don't want to add two seed is none checks in a row.

codecov · 2020-11-18T19:15:28Z

Codecov Report

Merging #4743 (93c231c) into master (f065ea6) will decrease coverage by 0%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #4743    +/-   ##
=======================================
- Coverage      93%     93%    -0%     
=======================================
  Files         135     135            
  Lines       10015    9873   -142     
=======================================
- Hits         9332    9181   -151     
- Misses        683     692     +9

pytorch_lightning/utilities/seed.py

tests/utilities/test_seed.py

pytorch_lightning/utilities/seed.py

carmocca · 2020-11-19T00:28:07Z

Hey @SeanNaren

I was a bit puzzled about this and checked out the branch. Changed the code to work. I do not have permissions to add commits to this PR so I created a branch on my fork off this branch. You should be able to git cherry-pick the commit here.

https://github.com/carmocca/pytorch-lightning/commit/eca9f4f8052c21dc14b272e1508aff71a79a9c84

Key changes:

Avoid calling select_seed_randomly from multiple places. Use the except for this.
Move warning to the except
Use warnings.warn instead of `logging.warning
Add tests for more cases

SeanNaren · 2021-01-07T17:01:38Z

cc @tchaton @awaelchli and @Borda for review! Should be good to go now

pytorch_lightning/utilities/seed.py

awaelchli · 2021-01-08T00:56:40Z

pytorch_lightning/utilities/seed.py

        seed = _select_seed_randomly(min_seed_value, max_seed_value)

+    log.info(f"Global seed set to {seed}")


this will log on each rank, is this desired?

Was going back and forth with myself about this, I think this is a good idea to ensure that the seed is set correctly on all processes, but I can be persuaded otherwise

I think it's fine :)

Borda · 2021-01-10T20:53:22Z

pytorch_lightning/utilities/seed.py

        seed = _select_seed_randomly(min_seed_value, max_seed_value)
+        rank_zero_warn(f"No correct seed found, seed set to {seed}")


Suggested change

seed = _select_seed_randomly(min_seed_value, max_seed_value)

rank_zero_warn(f"No correct seed found, seed set to {seed}")

seed_new = _select_seed_randomly(min_seed_value, max_seed_value)

rank_zero_warn(f"No correct seed `{seed}` found, seed set to `{seed_new}`")

seed = seed_new

#4743) * Check environment var independently to selecting a seed to prevent unnecessary warning message * Add if statement to check if PL_GLOBAL_SEED has been set * Added seed test to ensure that the seed stays the same, in case * if * Delete global seed after test has finished * Fix code, add tests * Ensure seed does not exist before tests start * Refactor test based on review, add log call * Ensure we clear the os environ in patched dict Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: chaton <[email protected]> (cherry picked from commit 635df27)

Check environment var independently to selecting a seed to prevent un…

dd26ea6

…necessary warning message

SeanNaren added the bug Something isn't working label Nov 18, 2020

SeanNaren added this to the 1.1 milestone Nov 18, 2020

SeanNaren requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, tchaton, teddykoker and williamFalcon as code owners November 18, 2020 16:21

SeanNaren mentioned this pull request Nov 18, 2020

seed_everything() runs _select_seed_randomly() even if PL_GLOBAL_SEED is set #4741

Closed

SeanNaren changed the title ~~[BUG] Check environ before selecting a seed to prevent warning message~~ [WIP] [BUG] Check environ before selecting a seed to prevent warning message Nov 18, 2020

SeanNaren added 2 commits November 18, 2020 16:52

Add if statement to check if PL_GLOBAL_SEED has been set

73badb4

Added seed test to ensure that the seed stays the same, in case

4528af9

SeanNaren changed the title ~~[WIP] [BUG] Check environ before selecting a seed to prevent warning message~~ [BUG] Check environ before selecting a seed to prevent warning message Nov 18, 2020

Borda reviewed Nov 18, 2020

View reviewed changes

pytorch_lightning/utilities/seed.py Outdated Show resolved Hide resolved

tests/utilities/test_seed.py Show resolved Hide resolved

if

f75d7f7

Borda reviewed Nov 18, 2020

View reviewed changes

pytorch_lightning/utilities/seed.py Outdated Show resolved Hide resolved

edenlightning modified the milestones: 1.1, 1.0.8 Nov 18, 2020

Delete global seed after test has finished

3c87893

carmocca reviewed Nov 18, 2020

View reviewed changes

pytorch_lightning/utilities/seed.py Outdated Show resolved Hide resolved

Borda modified the milestones: 1.0.8, 1.0.x Nov 20, 2020

edenlightning added this to the 1.1.x milestone Dec 8, 2020

mergify bot requested a review from a team December 12, 2020 14:58

SeanNaren and others added 2 commits January 7, 2021 17:00

Refactor test based on review, add log call

038d413

Merge branch 'master' into fix/4741-seed-init

dbcc9c3

awaelchli approved these changes Jan 8, 2021

View reviewed changes

Merge branch 'master' into fix/4741-seed-init

6fe921b

SeanNaren enabled auto-merge (squash) January 10, 2021 20:16

Borda approved these changes Jan 10, 2021

View reviewed changes

Borda requested a review from tchaton January 10, 2021 20:55

SeanNaren disabled auto-merge January 10, 2021 21:34

s-rog approved these changes Jan 11, 2021

View reviewed changes

Borda added the ready PRs ready to be merged label Jan 11, 2021

tchaton approved these changes Jan 11, 2021

View reviewed changes

SeanNaren and others added 2 commits January 11, 2021 13:18

Merge branch 'master' into fix/4741-seed-init

7e74e10

Ensure we clear the os environ in patched dict

34b6570

SeanNaren enabled auto-merge (squash) January 11, 2021 15:30

Branch was auto-updated.

93c231c

SeanNaren merged commit 635df27 into master Jan 12, 2021

SeanNaren deleted the fix/4741-seed-init branch January 12, 2021 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Check environ before selecting a seed to prevent warning message #4743

[BUG] Check environ before selecting a seed to prevent warning message #4743

SeanNaren commented Nov 18, 2020

SeanNaren commented Nov 18, 2020

carmocca commented Nov 18, 2020

SeanNaren commented Nov 18, 2020

codecov bot commented Nov 18, 2020 •

edited

Loading

carmocca commented Nov 19, 2020 •

edited

Loading

SeanNaren commented Jan 7, 2021

awaelchli Jan 8, 2021

SeanNaren Jan 8, 2021

awaelchli Jan 8, 2021

Borda Jan 10, 2021

		seed = _select_seed_randomly(min_seed_value, max_seed_value)

		log.info(f"Global seed set to {seed}")

		seed = _select_seed_randomly(min_seed_value, max_seed_value)
		rank_zero_warn(f"No correct seed found, seed set to {seed}")

-        seed = _select_seed_randomly(min_seed_value, max_seed_value)
-        rank_zero_warn(f"No correct seed found, seed set to {seed}")
+        seed_new = _select_seed_randomly(min_seed_value, max_seed_value)
+        rank_zero_warn(f"No correct seed `{seed}` found, seed set to `{seed_new}`")
+        seed = seed_new

[BUG] Check environ before selecting a seed to prevent warning message #4743

[BUG] Check environ before selecting a seed to prevent warning message #4743

Conversation

SeanNaren commented Nov 18, 2020

What does this PR do?

PR review

Did you have fun?

SeanNaren commented Nov 18, 2020

carmocca commented Nov 18, 2020

SeanNaren commented Nov 18, 2020

codecov bot commented Nov 18, 2020 • edited Loading

Codecov Report

carmocca commented Nov 19, 2020 • edited Loading

SeanNaren commented Jan 7, 2021

awaelchli Jan 8, 2021

Choose a reason for hiding this comment

SeanNaren Jan 8, 2021

Choose a reason for hiding this comment

awaelchli Jan 8, 2021

Choose a reason for hiding this comment

Borda Jan 10, 2021

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2020 •

edited

Loading

carmocca commented Nov 19, 2020 •

edited

Loading