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

Clarify what to override when subclassing random.Random #131244

Open
maple3142 opened this issue Mar 14, 2025 · 5 comments
Open

Clarify what to override when subclassing random.Random #131244

maple3142 opened this issue Mar 14, 2025 · 5 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@maple3142
Copy link

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.

@maple3142 maple3142 added the docs Documentation in the Doc dir label Mar 14, 2025
@vstinner
Copy link
Member

See also #84526.

@rhettinger
Copy link
Contributor

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.

@rhettinger rhettinger added type-bug An unexpected behavior, bug, or error and removed docs Documentation in the Doc dir labels Mar 18, 2025
@maple3142
Copy link
Author

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.

@picnixz picnixz added the stdlib Python modules in the Lib dir label Mar 18, 2025
@rhettinger
Copy link
Contributor

rhettinger commented Mar 18, 2025

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Status: Todo
Development

No branches or pull requests

5 participants