-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix: raise ValueError when provided seed is out-of-bounds #21029
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
fix: raise ValueError when provided seed is out-of-bounds #21029
Conversation
do you intend to make more changes, or should we mark it as |
Thanks for reviewing! I have one question to resolve: I have only changed the behavior to raise a The benefit of my current implementation is that it changes as little code as possible and mostly maintains the original behavior. The downside of my current implementation is that the behavior is different depending on whether or not the user uses the environment variable or the function argument. Having thought about this a bit more, I propose to change the behavior also for the case that the user specifies the seed via the environment variable. What do you think? |
sorry, I only looked at diff. Let's be consistent. Raise |
can you include test for it as well (with env variable) |
Thanks, I agree - just updated the code and test. |
Co-authored-by: Deependu <[email protected]>
Thanks for the feedback, @deependujha! Will now mark as ready. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21029 +/- ##
=======================================
- Coverage 87% 87% -0%
=======================================
Files 268 268
Lines 23303 23301 -2
=======================================
- Hits 20297 20295 -2
Misses 3006 3006 |
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.
Pull Request Overview
This PR changes the behavior of seed_everything
to raise a ValueError
when an invalid or out-of-bounds seed is provided, instead of falling back to a default seed value of 0 with a warning. This makes the function more strict and prevents misconfiguration when predictable seeding behavior is critical.
- Replace warning-based fallback behavior with strict error handling for invalid seeds
- Update error messages to be more descriptive and consistent
- Add comprehensive test coverage for the new validation behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/lightning/fabric/utilities/seed.py |
Updated seed validation logic to raise ValueError instead of warnings with fallback to 0 |
tests/tests_fabric/utilities/test_seed.py |
Updated existing tests and added new tests to verify ValueError is raised for invalid seeds |
src/lightning/fabric/CHANGELOG.md |
Added changelog entry documenting the breaking change |
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.
Thanks @jonathanking for the update! I agree with the intent here.
Curious to hear what @Borda, @SkafteNicki, and @matsumotosan have to say on this
Thanks @jonathanking for helping make |
* fix: crash when provided seed is out-of-bounds * fix: update handling of invalid seeds via PL_GLOBAL_SEED * fix: update test for invalid env var seed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: typo * Apply suggestions from code review Co-authored-by: Deependu <[email protected]> * changelog * update --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Deependu <[email protected]> (cherry picked from commit 8d847fd)
* fix: crash when provided seed is out-of-bounds * fix: update handling of invalid seeds via PL_GLOBAL_SEED * fix: update test for invalid env var seed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: typo * Apply suggestions from code review Co-authored-by: Deependu <[email protected]> * changelog * update --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Deependu <[email protected]> (cherry picked from commit 8d847fd)
What does this PR do?
Changes behavior of
seed_everything
to raise aValueError
if and only if the user provides a seed that is out of bounds ([0, 2**32-1]
) in order to more clearly match the user's expectations. Prevents misconfiguration of seeds when highly predictable behavior is desired.This behavior is changed whether the user specified the seed via the
seed_everything
function call or thePL_GLOBAL_SEED
env var.Fixes #21028
See also #18846 for context.
cc @deependujha
Breaking changes:
seed_everything
that is outside of[0, 2**32-1]
, the code will raise aValueError
where it previously changed the seed to0
.Before submitting
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:
Reviewer checklist
📚 Documentation preview 📚: https://pytorch-lightning--21029.org.readthedocs.build/en/21029/