Replaced rand_r with std:: random generation by lebeg · Pull Request #13574 · apache/mxnet

lebeg · 2018-12-07T10:38:31Z

Description

rand_r is not thread-safe and is not available on Android. Part of bigger #11148

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

larroy

good change, some comments.

larroy · 2018-12-07T11:25:27Z

        #pragma omp parallel for num_threads(omp_threads)
        for (int j = 0; j < T * N * H * D; j++) {
-          int rand_data = rand_r(&seed_);
+          static thread_local std::random_device device;


nice, is static needed when you use thread_local? https://stackoverflow.com/questions/22794382/are-c11-thread-local-variables-automatically-static

is it correct that we are seeding inside the loop?

Interesting link, thanks. I would still prefer the explicit way for clarity.

is it correct that we are seeding inside the loop?

Since it's static we doing it only once.

For every thread.

larroy · 2018-12-07T11:27:01Z

      #pragma omp parallel for num_threads(omp_threads)
      for (int i = 0; i < T * N * I; i++) {
-        int rand_data = rand_r(&seed_);
+        static thread_local std::random_device device;


could we wrap this up in a function? Or would interfere with thread_local? Seems we are duplicating code.

I'm not sure where to put it, do you have any suggestions?

larroy

LGTM

perdasilva · 2018-12-09T17:29:03Z

+  static thread_local auto dice = std::bind(distribution, generator);
+  return dice();
 }



Could you add a RandInt method and use this in rnn_impl?
Would be interested to get understand what happens memory-wise with these thread_locals, and also the openmp parallelization when I see you next =)

The problem is file is in tests/.

There's no facepalm emoticon, so I had thumbs-up

Roshrini · 2018-12-21T21:39:28Z

@perdasilva @larroy Is your concern answered? Is this PR good to go?

Roshrini · 2019-01-02T19:12:59Z

@mxnet-label-bot Update [pr-awaiting-response]

anirudhacharya · 2019-01-14T22:55:13Z

@lebeg @larroy @perdasilva ping for update

lebeg · 2019-01-15T17:32:20Z

I think this PR is good to merge, can somebody do this?

lebeg · 2019-01-15T17:32:41Z

@mxnet-label-bot Update [pr-awaiting-merge]

sandeep-krishnamurthy · 2019-01-28T17:52:56Z

@apeforest - Can you please take a look at this PR?
Thanks @lebeg for this PR.

perdasilva · 2019-01-29T07:56:22Z

@anirudhacharya @lebeg all good from my side

lebeg · 2019-02-11T21:41:42Z

Can anybody merge this?

lanking520 · 2019-02-11T21:53:50Z

@apeforest Could you please take a look?

szha

Given that we have random number generator as operator resources, the thread local random number generator inside CPU RNN operator looks like a hack to me.

eric-haibin-lin · 2019-02-12T23:52:06Z

        #pragma omp parallel for num_threads(omp_threads)
        for (int j = 0; j < T * N * H * D; j++) {
-          int rand_data = rand_r(&seed_);
+          static thread_local std::random_device device;


This should probably addressed at the framework level by providing APIs to get rand numbers. We should not expect developers who implement operators to handle thread local variables

@eric-haibin-lin In general, I don't see why a thread local variable is an issue - there is a separate generator for every thread and nothing needs to be done additionally to ensure thread safety. Otherwise locking needs to be in place.

I could add a generate random number function to the framework API, but wouldn't change the implementation. What do you think?

And in case of 1 function for all there will be complications with setting the seed if needed as well.

suggest to refer to RandGenerator<xpu> as I mentioned below. It handles thread-safe problem without locking. The implement is somewhat tricky though.

lebeg · 2019-02-13T16:58:33Z

we have random number generator as operator resources

@szha can you add a bit more information on this? Do you mean there is a random number generator operator?

szha · 2019-02-13T18:26:28Z

@lebeg I recommend looking at the dropout implementation (in src/operator/nn/dropout-inl.h)

TaoLv · 2019-02-14T02:03:33Z

Looks like the change is extracted from #11148 . Reviewed the code before and related comment and response are here: https://github.com/apache/incubator-mxnet/pull/11148/files#r210174235

lebeg · 2019-02-14T22:46:33Z

@szha @TaoLv thanks, quoting here again for reference:

Well, the problem is not about the seed itself, but with usage of rand_r and the fact that it's multithreaded. As far as I can tell in dropout-inl.h#L85 the seed is set outside of OpenMP threads and no random number gets generated with it afterwards, so it should be fine.

yzhliu · 2019-02-15T06:42:52Z

@lebeg Can we port it to https://github.com/apache/incubator-mxnet/blob/master/include/mxnet/random_generator.h ? It is designed exactly for non-threadsafe rand number generators (in which cuda device random api is not thread-safe). I guess this is what @szha meant.

Ref:
https://github.com/apache/incubator-mxnet/blob/master/include/mxnet/random_generator.h
https://github.com/apache/incubator-mxnet/blob/master/src/resource.cc#L298
https://github.com/apache/incubator-mxnet/blob/master/src/operator/random/sampler.h

yzhliu · 2019-02-15T07:12:38Z

Usually using thread_local rand generator is not a good idea, not only in the sense of system design, but it could be invalid.

Say if you use 4 threads to fill an array A[4], each Gaussian random generator fills one entry with its head element - A[0] = gen1[0], A[1] = gen2[0], A[2] = gen3[0], A[3] = gen4[0] - this array in fact does not necessarily follow Gaussian distribution (sometimes with specific implement it does, but no guarantee). Pseudorandom sequence has to be long enough in order to satisfy the distribution property. Such "bug" is rare but will be extremely hard to debug once occurs (which I met before).

Also it will be great to let users set their global seed, as some (maybe not-well-designed) algorithms are sensitive to seed number. This means all random generators within the system should take same input seed as their constructor argument. (I know currently this rule is not well followed in mxnet though.)

This is why RandGenerator is implemented so complicated.

lebeg · 2019-02-15T09:07:01Z

Thank you @yzhliu for your input! Now it's indeed much clearer to me. I will take a look at the RandGenerator implementation and modify the change accordingly.

vandanavk · 2019-03-07T20:31:54Z

@mxnet-label-bot update [pr-awaiting-response, Operator]

karan6181 · 2019-03-19T00:30:40Z

@lebeg Did you get a chance to look at the RandGenerator implementation ? Can you please make the relevant code changes for that? Thank you!

lebeg · 2019-03-26T09:54:43Z

Closing this for now, no capacity to work on this right now.

Replaced rand_r with std:: random generation

1929b42

lebeg requested a review from anirudh2290 as a code owner December 7, 2018 10:38

larroy reviewed Dec 7, 2018

View reviewed changes

larroy approved these changes Dec 7, 2018

View reviewed changes

nswamy added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-review PR is waiting for code review labels Dec 8, 2018

perdasilva reviewed Dec 9, 2018

View reviewed changes

marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-review PR is waiting for code review labels Jan 2, 2019

marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Jan 15, 2019

larroy approved these changes Feb 6, 2019

View reviewed changes

szha requested review from TaoLv and yzhliu February 12, 2019 23:26

szha suggested changes Feb 12, 2019

View reviewed changes

eric-haibin-lin reviewed Feb 12, 2019

View reviewed changes

marcoabreu added Operator pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Mar 7, 2019

lebeg closed this Mar 26, 2019

Conversation

lebeg commented Dec 7, 2018

Description

Checklist

Essentials

Changes

Comments

Uh oh!

larroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larroy left a comment

Choose a reason for hiding this comment

Uh oh!

perdasilva Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roshrini commented Dec 21, 2018

Uh oh!

Roshrini commented Jan 2, 2019

Uh oh!

anirudhacharya commented Jan 14, 2019

Uh oh!

lebeg commented Jan 15, 2019

Uh oh!

lebeg commented Jan 15, 2019

Uh oh!

sandeep-krishnamurthy commented Jan 28, 2019

Uh oh!

perdasilva commented Jan 29, 2019

Uh oh!

lebeg commented Feb 11, 2019

Uh oh!

lanking520 commented Feb 11, 2019

Uh oh!

szha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lebeg commented Feb 13, 2019

Uh oh!

szha commented Feb 13, 2019

Uh oh!

TaoLv commented Feb 14, 2019

Uh oh!

lebeg commented Feb 14, 2019

Uh oh!

yzhliu commented Feb 15, 2019

Uh oh!

yzhliu commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

perdasilva Dec 9, 2018 •

edited

Loading

yzhliu commented Feb 15, 2019 •

edited

Loading