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

Random module deprecations and stabilization improvements #23752

Merged

Conversation

jeremiah-corrado
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado commented Nov 1, 2023

Stabilization changes to the Random module:

  • Deprecates RandomStreamInterface
  • Deprecates the RNG, defaultRNG and RandomStream symbols used to select between PCG and NPB rngs
  • Deprecates standalone methods that accept an RNG argument in favor of those that don't
  • Adds a new record, randomStream which wraps the PCGRandomStream implementation in a slightly simplified interface:
    • a randomStream is now type-specific (can only generate values of one type)
    • a randomStream is not parallel safe
    • fillRandom is now called fill
  • Deprecates PCGRandom submodule in favor of usingrandomStream
  • Moves the NPBRandom submodule to a standalone package module
  • Deprecates createRandomStream in favor of new randomStream
  • Deprecates SeedGenerator (and the RandomSupport submodule in general)

The Random module is no longer unstable as a whole. The following symbols are unstable pending further design discussions (summarized here):

  • permutation and randomStream.permutation
  • randomStream.choice
  • randomStream.getNth and randomStream.skipToNth
  • randomStream.iterate
  • The PCGRandomLib submodule
  • methods that internally generate a random seed value

Notes to reviewer

  • A majority of the changes in this PR are adjustments to tests to use randomStream instead of PCGRandomStream or NPBRandomStream
    • in most cases randomStream is used (which sometimes involved adjusting .good files when moving from the NPB algorithm)
    • in a few other cases the NPB algorithm is used via the NPBRand package module. This was only done for some performance tests that rely on NPB's relative speed, or in cases where adjusting the good file would not have been trivial.
  • Note that many of the tests that use Random are not testing the module itself, but are using it to create random test data or for other algorithms that require randomness (i.e., some sorting algorithms).
    • Only the new tests under deprecated/ or under library/standard/Random/ are testing the module itself and thus should probably be reviewed more closely

Testing:

  • local paratest (gnu)
  • local paratest (llvm)
  • gasnet paratest
  • inspected docs

jeremiah-corrado and others added 26 commits October 27, 2023 16:09
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
…dom module

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
…dule

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
…). Remove the parSafe option from randomStream, making it always parallel unsafe

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@jeremiah-corrado jeremiah-corrado marked this pull request as ready for review November 20, 2023 21:07
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Copy link
Member

@mppf mppf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the test changes yet, but wanted to get my review comments about the module changes to you before I get to the test changes.

doc/rst/meta/modules/packages.rst Outdated Show resolved Hide resolved
modules/packages/NPBRand.chpl Outdated Show resolved Hide resolved
modules/packages/SortedSet/Treap.chpl Outdated Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
modules/standard/Random.chpl Show resolved Hide resolved
modules/standard/Random.chpl Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
modules/standard/Random.chpl Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
…precated manner

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
…tion/testGenLib related to importing 'Set' and 'NPBrandom' in 'Random' (use an associative domain instead of a set). Reproducer and issue forthcoming.

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Copy link
Member

@mppf mppf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm worried that the tests moved to test/deprecated/random were moved with too big of a brush. Many of these are useful tests of determinism or statistical properties and I think we want to keep those tests. But, I see that many of these tests might use a deprecated feature. If you don't feel you have time to update them before merging this PR, that is OK, but I'd like to see follow-up to move them back to test/library & potentially have the .good files include the deprecation warning until we update them individually to avoid deprecated features.

modules/standard/Random.chpl Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
modules/standard/Random.chpl Outdated Show resolved Hide resolved
test/chplvis/benchmarks-hpcc/fft-vdb.chpl Outdated Show resolved Hide resolved
modules/packages/NPBRandom.chpl Show resolved Hide resolved
test/deprecated/random/checkWriteThis.good Outdated Show resolved Hide resolved
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
… calling the time module directly

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@jeremiah-corrado
Copy link
Contributor Author

jeremiah-corrado commented Nov 22, 2023

Thanks for the review @mppf!

Based on your comments above, I'll have two follow up PRs:

  • one to update the tests in library/standard/Random/ that have deprecation warnings in their good files. I'll make those use the undeprecated symbols and add new corresponding tests back to /deprecated/random/ as needed.
  • another to create a future for the compiler bug I worked around in 6076ab9 (I'll also make an issue for that)

@jeremiah-corrado jeremiah-corrado merged commit a612715 into chapel-lang:main Nov 22, 2023
7 checks passed
@jeremiah-corrado jeremiah-corrado deleted the random-stabilization-changes branch November 22, 2023 17:12
jeremiah-corrado added a commit that referenced this pull request Nov 22, 2023
Fix a few test failures in GPU and linear algebra testing caused by
#23752.

Also updates a few other tests that used deprecated Random features (but
weren't failing due to NOTEST's etc.)

- [x] failing gpu passing (CHPL_GPU=cpu)
- [x] failing linalg tests passing on cray xc
- [x] paratest

[ trivial - not reviewed ]
jeremiah-corrado added a commit that referenced this pull request Nov 27, 2023
A follow up to #23752

- fixes a few tests that were failing on 32 bit linux
- evidently the PCG random number generator will produce floating point
values that are more prone to floating point rounding errors on 32 bit
systems (as compared to the NPB random number generator).
   - the tests have been adjusted to use NPB instead of PCG
- removes deprecated symbols from some random module tests and adjusts
good files
- follow up to [this
comment](#23752 (comment))

**Testing:**
- [ ] failing tests passing on 32 bit linux
- [x] adjusted tests passing
- [x] paratest

[ trivial - not reviewed ]
jeremiah-corrado added a commit to jeremiah-corrado/chapel that referenced this pull request Nov 28, 2023
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
jeremiah-corrado added a commit that referenced this pull request Nov 28, 2023
#23752 broke `reductions/thomasvandoren/test/TestSorted` by replacing
the test's .good file with its .bad file. This PR corrects that mistake.

Also adds a new future to track a bug related to separate compilation
discovered while implementing changes to the Random module. See:
#23929

- [x] clean match against .bad in
reductions/thomasvandoren/test/TestSorted
- [x] clean match agianst .bad in new future
- [x] paratest

[ trivial - not reviewed ]
@bradcray
Copy link
Member

bradcray commented Dec 8, 2023

Hi @jeremiah-corrado and @mppf — I didn’t catch this until now, but I don’t think the HPCC benchmarks should use NPBRandom, but Random instead. The only tests that I’m aware of that care what the random number generator is are the NAS Parallel Benchmarks (where the NPB comes from). All others should focus on using the default/standard random number generator instead to be less "weird" / rely on unstable packages and fetures. For example, stream just wants some data in the vectors, and doesn’t really care what that data is.

Can you take a cleanup item (post-release at this point) to update such cases to simply use Random? If you run into surprises or cases that seem to contradict what I'm saying, please let me know.

@jeremiah-corrado
Copy link
Contributor Author

Yeah, I can create a follow-on PR next sprint that makes those changes.

jeremiah-corrado added a commit that referenced this pull request Dec 19, 2023
Remove uses of `NPBRandom` from tests and replace with `Random`, per the
suggestion
[here](#23752 (comment)).

The tests that still use NPBRandom are:
* the NAS parallel benchmarks
* tests specific to the NPBRandom module (deprecation tests, correctness
tests, etc.)
* some constrained-generics tests that have their own copies of the
Random/NPBRandom modules
* the machine learning test that rely on it's relative speed
* the `ssca2` tests that rely on some lower level NPBRandom features
* a few miscellaneous tests use `NPBRandom.oddTimeSeed` to get a seed
value since `Random` doesn't have a stable way to do that yet

Testing:
- [x] local paratest
- [x] gasnet paratest

[ reviewed by @jabraham17 ] - Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants