keras.utils.set_random_seed clear the global SeedGenerator.
#21874
Conversation
This is needed to get reproducible results with `keras.random` ops.
Summary of ChangesHello @hertschuh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with reproducibility in keras.random operations by ensuring the global SeedGenerator is cleared when keras.utils.set_random_seed is invoked. The implementation is sound, and the new test case provides good coverage for the fix. I have a couple of suggestions: one to improve maintainability by replacing a magic string with a constant, and another to address a potential test failure on the NumPy backend.
| reason="Numpy backend does not support random seed setting.", | ||
| ) | ||
| def test_set_random_seed(self): | ||
| def test_set_random_seed_with_seed_generator(self): |
There was a problem hiding this comment.
This test was previously skipped for the NumPy backend. By removing the skipif decorator, this test will now run for the NumPy backend. However, the get_model_output function uses tf.data.Dataset, which is specific to TensorFlow and will likely cause the test to fail when run with the NumPy backend.
To ensure this test can run across all backends, you could modify get_model_output to not use tf.data.Dataset. For example:
def get_model_output():
model = keras.Sequential(
[
keras.layers.Dense(10),
keras.layers.Dropout(0.5),
keras.layers.Dense(10),
]
)
x = np.random.random((32, 10)).astype("float32")
return model.predict(x)Alternatively, if the intention is to keep this test for TensorFlow-based backends only, the skipif decorator should be restored.
| # Store seed in global state so we can query it if set. | ||
| global_state.set_global_attribute(GLOBAL_RANDOM_SEED, seed) | ||
| # Remove global SeedGenerator, it will be recreated from the seed. | ||
| global_state.set_global_attribute("global_seed_generator", None) |
There was a problem hiding this comment.
To improve maintainability and avoid using 'magic strings', consider defining "global_seed_generator" as a module-level constant, similar to how GLOBAL_RANDOM_SEED is handled. This would make the code easier to read and maintain.
For example, you could add at the top of the file:
GLOBAL_SEED_GENERATOR_KEY = "global_seed_generator"And then use it here:
global_state.set_global_attribute(GLOBAL_SEED_GENERATOR_KEY, None)Ideally, this constant would be shared with keras/src/random/seed_generator.py to ensure consistency.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #21874 +/- ##
==========================================
- Coverage 82.57% 76.58% -6.00%
==========================================
Files 577 577
Lines 59586 59587 +1
Branches 9347 9347
==========================================
- Hits 49205 45632 -3573
- Misses 7975 11486 +3511
- Partials 2406 2469 +63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
This is needed to get reproducible results with
keras.randomops.