Skip to content
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

Feature/12804 Automatic Seeding of the CLI #12822

Merged
merged 24 commits into from
Apr 28, 2022

Conversation

Schinkikami
Copy link
Contributor

@Schinkikami Schinkikami commented Apr 20, 2022

What does this PR do?

The argument seed_default_value of the LightningCLI is changed to the type Optional[Union[bool,int]] = True (from Optional[int] = None). The Optional flag is set for backwards compatibility. Setting it to None has the same effect as setting it to False, but raises a rankzero - LightningDeprecationWarning.

If the value is set to True a valid (range np.iiinfo(np.int32).min to np.iiinfo(np.int32).max ) seed is drawn at random and added as default to the seed_everything argument.

The issue was discussed and approved in 12804. The type of --seed_everything is set to Optional[int32], as I do not think, that setting it to Optional[Union[bool,int]] is required. By default the CLI now chooses a new seed with every instantiation, which can of course be overwritten with --seed_everything from the CLI or a config file. The only time, one would like to set it to False, would be if somewhere in the code the seed is already set, but in this case the automatic seeding should be disabled in the code (by setting seed_default_value to False) and not in a config file with --seed_everything.

Choosing a seed outside of the valid seed range, raises and error and avoids unnoticed accidental mistakes.

The version in the deprecation warning still needs to be set.

Fixes #12804
For a motivation and discussion, have a look at the issue.

Does your PR introduce any breaking changes? If yes, please list them.

No.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the 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

Did you have fun?

Make sure you had fun coding 🙃

@carmocca carmocca added feature Is an improvement or enhancement lightningcli pl.cli.LightningCLI labels Apr 20, 2022
@carmocca carmocca added this to the 1.7 milestone Apr 20, 2022
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
tests/utilities/test_cli.py Outdated Show resolved Hide resolved
tests/utilities/test_cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@carmocca carmocca added the community This PR is from the community label Apr 22, 2022
@carmocca carmocca self-assigned this Apr 22, 2022
tests/utilities/test_cli.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tests/utilities/test_cli.py Show resolved Hide resolved
pytorch_lightning/utilities/cli.py Show resolved Hide resolved
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Apr 25, 2022
pytorch_lightning/utilities/cli.py Outdated Show resolved Hide resolved
tests/deprecated_api/test_remove_1-9.py Outdated Show resolved Hide resolved
@carmocca carmocca enabled auto-merge (squash) April 27, 2022 14:19
@carmocca carmocca merged commit a62c227 into Lightning-AI:master Apr 28, 2022
@Schinkikami
Copy link
Contributor Author

Hey, I was just thinking about the pull_request again.
In my original pull_request, the default value was set to "False", but during the discussion it was suggested to change it to "True", which I then did. Wouldn't that be a breaking change? If a user used to manually set the seed before calling the CLI (I mean he should have used the seed_everything_default argument anyway...), the CLI now automatically and by default overwrites his seed. I can see that leading to funny debug sessions for people using the CLI. Or is that okay, as they should have passed the value to the CLI anyway?

@carmocca
Copy link
Contributor

You are correct @Schinkikami. I opened #13110 with a fix.

@adosar
Copy link
Contributor

adosar commented Mar 25, 2024

@carmocca Does --seed-everything includes workers==True?

@carmocca
Copy link
Contributor

@adosar Yes:

if config_seed is True:
# user requested seeding, choose randomly
config_seed = seed_everything(workers=True)
else:
config_seed = seed_everything(config_seed, workers=True)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR is from the community feature Is an improvement or enhancement lightningcli pl.cli.LightningCLI ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic seeding when using the LightningCLI
6 participants