Nice docs! Thank you!

This class is CPU only in this PR, is that right?

the torch.Generator() api is CPU only in this PR (also it's cpu only in master as well):
https://github.com/pytorch/pytorch/blob/2b826c6171608bebd96be6dd1f8201ab6a0a8dff/torch/csrc/Generator.cpp#L47-L69
In the second PR, I want to be able to create CUDAGenerator when I pass the device="cuda" in torch.Generator()

OOC, where did we get this number from?

So since the Philox engine has two 32-bit keys (that is it's state is 64 bit I think?), it is recommended that a RNG engine should be seeded with the same number of bits as its state (I think I read this somewhere in one of the links I posted in the last PR). Also the seed value should have a good distribution of 0s and 1s in the bit representation. So I chose a large prime number that can be fit into uint64_t: https://primes.utm.edu/curios/page.php/67280421310721.html. You'll notice another number in:
https://github.com/pytorch/pytorch/blob/2b826c6171608bebd96be6dd1f8201ab6a0a8dff/test/common_utils.py#L46-L48
python side won't let me seed with 64 bit? So I just chose a large prime there under 32 bit.
¯_(ツ)_/¯

This copy constructor seems pretty dangerous, because you're planning to subclass this class, and add fields to the subclasses. When you allow copy construction of classes with subclasses, you are exposing yourself to the possibility of object slicing (https://stackoverflow.com/questions/274626/what-is-object-slicing). It is probably best if we don't allow copy constructor at all, and instead have a virtual method for doing clones.

So, we should disable all copy/move constructors/assignment on the superclass Generator.

CC @smessmer

it's not an object slicing issue as long as you take other per reference, but disabling copy constructors where not needed is always a good idea.

nit: mutex_

This looks unnecessary. I'd be very happy to say that device_ is immutable and therefore does not need to be locked.

I think we need to think more clearly about what thread-safety guarantees we want this class to give. This is important because you've made device_ and current_seed_ protected variables, which means that people who write subclasses can access these directly (without going through the locked getter); if you don't give clear guidance about thread safety expectations, they will probably get it wrong.

My understanding (which could be wrong) is that the most important thing to make thread safe is the random number generator state itself; changes to device or current seed don't actually happen concurrently from different threads.

That is an important point about protected variables that I completely overlooked! You are right that the most important thing to lock is the rng. I see this pattern that people like to call torch.manual_seed() before they use their rand functions. So I thought if that would raise thread safety issue if I don't lock the getters and setters as well.

I feel like usually manual seed is called at the beginning of the program, or from a single threaded context. So that's why I hypothesize it's OK not to lock. Although it's going to setup the RNG state, which we should lock just for good reason, so maybe not.

TBH, the fact that we are sharing RNG state across threads is kind of crazy, it really should be a thread local concept. But I agree this is consistent with what we did previously, so...

Do we ever instantiate a Generator (and not a subclass)? If not, we should just declare this pure virtual and remove the stub immediately. If that's not possible, I'd prefer if this raised an error if it was called at runtime, rather than just ignore the seed parameter.

I would also support making Generator as pure virtual but got confused when all the CPU and CUDA methods (manualSeedAll) were put in there (may be people didn't wanna do dynamic_cast and call subclass's specialized method directly from Generator*/Generator&?). But either way, I didn't see an instance where we create Generator object, but we just pass Generator* in function signatures, which is then dynamic_cast'ed to the specific backend the code needs.

Fortunately for you, this is in return position, and C++ supports covariant return types. This means you can override the method with a refined return type (CPUGenerator) and it is still a valid override.

nit: Just drop the detail block below the class, then you don't need a forward declaration anymore

This looks wrong, you shouldn't declare variables in headers. Move these to the cpp file.

Are these really internal? You call them from a lot of places... what's the normal way to get one of these generators?

So ideally I wanted to do the following from Context.h:

auto getDefaultGenerator(at::CPU) -> gives me CPUGenerator
auto getDefaultGenerator(at::CUDA) -> gives me CUDAGenerator
auto getDefaultGenerator(at::device({at::kCUDA, 1}) -> gives me CUDAGenerator at device index 1
... same for createGenerator

I thought I could hack this behavior using SFINAE (similar to what I did in the PhiloxRNGEngine's operator() call) but ended giving up on it after not being able to fix template errors. So now I am creating this API inside these subclasses, such that when somebody needs a CPUGenerator, they can just call this instead of getDefaultGenerator which returns a Generator&. All this to avoid having to dynamic cast all the time in the back end, when needing specialized class methods (such as random(), random64() methods from the CPUGenerator()).

This sounds fine, maybe they shouldn't be internal ;)

We should resolve the question of thread-safety first (though I agree that copies should be mutex protected), but I do wonder about how much this idiom of taking lock_guard as an argument is giving us. Is this a recommendation somewhere? :)

I was just paranoid about thread safety and just followed this: https://www.justsoftwaresolutions.co.uk/threading/thread-safe-copy-constructors.html. Also just wanted copy constructors but having the mutex wouldn't give me auto generated ones, so did the whole google search how to get my copy constructors thread safe.

I guess this is moot since we're killing all the copy constructors

So, eventually, are we going to put CPUGenerator into the generator registry? We certainly can't keep doing the current strategy (if-statements) when CUDA rolls around, because you won't be allowed to access that symbol from here.

So the problem with generator_registry is, CUDAGenerator will have its own std::vector of default generators, where the size of std::vector is the number of GPUs in the machine. Currently size of generator_registry is the number of backends in PyTorch. Hence, For the symbols, I was planning on the doing the if statements and call at::getCUDAHooks().getCUDAGenerator(device). Wouldn't that work?

That would work, I suppose, but in that case we might as well just give up on the generator registry and do this entirely through hooks.

We can't assume RTTI is available on all platforms we support. You should add a tag to the Generator superclass and use that to test for the types in question.

nit: use c++ static_cast here

I haven't checked these algorithms yet

I missed this on my first review, but could we have this return a std::unique_ptr<CloneableGenerator<CPUGenerator, Generator>> instead of a raw pointer that is manually memory managed? If you want to do this in a follow-up PR, that's OK (if this is the only problem I'll initiate land without a fix.)

This pointer gets wrapped by a unique pointer: https://github.com/pytorch/pytorch/pull/16604/files#diff-621abddec4e628a1cd833e601c92751eR14 . It has to remain a raw pointer to support covariant unique_ptr return type in my understanding: https://www.fluentcpp.com/2017/09/12/how-to-return-a-smart-pointer-and-use-covariance/

Oh, I forgot about that. Disregard!

I'm actually not sure we need to maintain BC here, but since you've done so, I'll take it. We can always break BC later.

Oh, I see, you've implemented a limited form of BC, where you can load old checkpoints, but saved checkpoints are incompatible with new versions.

Yes, I wanted to support loading of old checkpoints in set_rng_state, however, get_rng_state would return a THGeneratorNew as a byte tensor.

What happened to this comment?

This comment will come back once @fritzo and @neerajprad gets a chance to investigate why changing initialization scheme of mt19937 breaks test_distributions.py

I can think of plausible reasons for the changes here, but since there isn't commentary about it in comments anywhere, I'm left guessing. I think the changes are OK, but can you spell out why?

This is because of supporting BC set_rng_state. Since THGeneratorState has all the members of the mt19937, I needed to copy in the raw array and other members. Hence, created a constructor and std::array was the way to go to have it as a parameter in the constructor.

Your editor is dropping trailing newlines again XD

Unfortunately, c10::optional is not POD. You'll have to split them into separate float and bool fields.

Also, if you want to assert something is POD, best to do a static assert, e.g., static_assert(std::is_pod<Params>::value, "Params is not POD");

This looks dangerous to me. If you ever modify THGeneratorStateNew so that its size coincides with THGeneratorState, you have a really nasty bug on your hand where we'll read in the RNG from the wrong direction.

I suppose I could be convinced that this situation is incredibly unlikely to happen, since it's not every day you're going to muck around with the rng state. But at the very least, this warrants a static assert that the sizeof THGeneratorState doesn't match sizeof THGeneratorStateNew

If you are serious about catching these errors, we need some sort of header that we can use to identify the format of the RNG state.

I didn't catch this previously, but this now looks a little skeevy to me. Can you guarantee that we actually have a CPUGenerator at this point in time? If we can't guarantee it, you better have a check.

Does this actually give you identical behavior to the old sampling behavior? I'm not convinced, because you only set double_normal_sample here, and not float_normal_sample, but I imagine the old state was utilized both for float and double sampling. (I don't think there is necessarily anything wrong with this, but when it comes to BC guarantees, we should call a spade a spade, and say what we are actually preserving, and what we are not. What we seem to have here is "best effort" BC--we won't crash, but the random numbers won't necessarily be the same.)

Yes, we would get identical behavior, since I actually haven't utilized the float versions that I'm storing and have used at::normal_distribution<double> throughout (since that's what being done in TH right now, THRandom_normal returns a double, and is utilized for both float and double dispatch).

It would be nice to put more diagnostic info here: how big was the RNG state, and what were the valid sizes?