Clarify what to override when subclassing random.Random

[maple3142]

Documentation

In current documentation about the random.Random class, it said that overriding getrandbits() is optional, so it may make people think just overriding random() is sufficient to control the behavior of the class. But in practice methods like randbytes use getrandbits, which is implemented in C to get randomness from MT19937. So it has the following behavior:

import random


class CustomRand(random.Random):
    def random(self):
        print("CustomRand.random()")  # does not get called
        return 0.5


myrand = CustomRand()
myrand.seed(123)  # just for demonstrating that randbytes still use MT19937
r1 = myrand.randbytes(4)

random.seed(123)
r2 = random.randbytes(4)

print(r1 == r2)  # True

Which shows that overriding random() only is not sufficient. From the issue #84466, this seems to be the expected behavior (or not?). So I think this should be documented that overriding getrandbits() is not optional.

[vstinner]

See also #84526.

[rhettinger]

The docs are correct. It is the implementation that is buggy. The intent and promise was to make getrandbits optional because some rng's, Wichmann–Hill for example, only have a random method. The entire point of the __init_subclass__ method is to guarantee that _randbelow adapts correctly to an optional getrandbits.

This was true (and likely relied upon) until randbytes was added. That method should have called _randbelow rather than getrandbits. Subsequent to the randbytes implementation error, I incorrectly added a comment in 2020 saying the downstream random methods were allowed to call random, getrandbits, or _randbelow. That comment should have only listed random and _randbelow. Calling getrandbits directly violates both the design and the documented promise of the module.

[maple3142]

Okay, so randbytes is expected to depends on _randbelow, which can reuse random if only it is available.
But what about getrandbits? Is it expected to use the underlying MT19937 or it should be supported by random.

[rhettinger]

But what about getrandbits?

That is the issue that Victor referenced above and it is also a problem. The inherited getrandbits is inconsistent with the user override of random. So getrandbits could either be blocked (perhaps raising NotImplementedError) or could get a default implementation based on _randbelow.

I suspect (but don't actually know) this rarely arises in practice. Subclassing isn't common (most people use MT19937 right out of the box). When they do subclass, they tend to add getrandbits because it is often easy and because it significantly extends the range of the integer functions. And even if they didn't implement getrandbits, they still won't notice because randbytes is not a popular method (the secrets version is preferred). That said, all this should be fixed so that the module is in a coherent state that matches the documentation.

[rhettinger]

@tim-one What do think about automatically adding getrandbits to subclasses that don't supply one. Perhaps something like this:

def _getrandbits_based_on_random(self, n):
    result = 0
    bits = 0
    while n > bits:
        result = (result << 53) | _floor(self.random() * 2.0**53)
        bits += 53
    return result >> (bits - n)

This could be added by __init_subclass__ when needed. That would fix this bug and the one referenced by Victor. It would better match the documentation. And possibly _randbelow_without_getrandbits would no longer be needed.