Make tests more deterministic #17008
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
bot
added
Python
galipremsagar
added
improvement
github-actions
bot
added
Python
|
If interested, I included this pre-commit check in pandas to check that https://github.com/pandas-dev/pandas/blob/main/.pre-commit-config.yaml#L210-L211 |
|
I like the idea of a pygrep pre-commit check. We could also look out for |
| @@ -245,7 +245,7 @@ def hash_vocab( | |||
| """ | |||
| Write the vocab vocabulary hashtable to the output_path | |||
| """ | |||
| np.random.seed(1243342) | |||
There was a problem hiding this comment.
@VibhuJawa Switching to default_rng changes the random values generated and thus updates to python/cudf/cudf/tests/data/subword_tokenizer_data/bert_base_cased_sampled/vocab-hash.txt have been made, will that be an issue for tokenizer?
There was a problem hiding this comment.
I dont think it should be , will have to test with some common vocabulary files to be sure
| @@ -95,6 +95,18 @@ repos: | |||
| entry: 'pytest\.xfail' | |||
| language: pygrep | |||
| types: [python] | |||
| - id: no-unseeded-default-rng | |||
There was a problem hiding this comment.
Is there a preference to keep these checks separated? Seems like we could do something similar to the check that @mroeschke linked in consolidating all of these entries into a single check that runs against all Python files at once
There was a problem hiding this comment.
FWIW, I quite like the separate entries, since the name of the entry gives some information as to what went wrong. If the regex gets complicated, I find it hard to see what's going on. But I don't have strong feelings here.
| @@ -95,6 +95,18 @@ repos: | |||
| entry: 'pytest\.xfail' | |||
| language: pygrep | |||
| types: [python] | |||
| - id: no-unseeded-default-rng | |||
There was a problem hiding this comment.
FWIW, I quite like the separate entries, since the name of the entry gives some information as to what went wrong. If the regex gets complicated, I find it hard to see what's going on. But I don't have strong feelings here.
| np.random.seed(12) | ||
| rng = np.random.default_rng(seed=0) |
There was a problem hiding this comment.
I suppose it doesn't really matter, but why the change of seed?
There was a problem hiding this comment.
It didn't actually matter keeping the seed same either:
In [1]: import numpy as np
In [2]: np.random.seed(0)
In [3]: np.random.randint(0, 10)
Out[3]: 5
In [4]: np.random.default_rng(0).integers(0, 10).item()
Out[4]: 8
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_stumpy_distributed.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_stumpy_distributed.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Charles Blackmon-Luca <[email protected]>
…into numpy_random
|
@charlesbluca @wence- I addressed all your reviews. This should be ready for review now. |
Description
Fixes #17045
This PR removes randomness in our pytests and switches from using
np.random.seedtonp.random.default_rngin all of the codebase.Checklist