[issue40346] Redesign random.Random class inheritance

[Tim Peters]

Tim Peters  added the comment:

This wasn't written with subclassing in mind to begin with.  A class was just 
an obvious way to allow advanced users to construct instances with their own 
states (e.g., so they could build pseudo-independent generators for parallel 
programming).

When SystemRandom got added, I believe that did about as little as possible 
just so that it "would work".

Making it easy to create subclasses was never a goal regardless.

I don't mind if that _becomes_ a goal, but with some disclaimers:

- If that makes any method currently in use slower, it's not worth it.

- Saving bytes in a SystemRandom instance is of no practical value.

So I think I'm agreeing with Raymond that reworking this doesn't solve any 
actual problem.  But I'm not opposed to making changes for aesthetic reasons, 
provided such changes don't impose costs beyond the always-present risk of 
breaking things with any change.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[Raymond Hettinger]

Raymond Hettinger  added the comment:

-1   I am opposed to this redesign.  There is no problem being solved that 
warrants changing a long standing public API.  It is irresponsible to just 
guess at whether anyone is using the API.  To add randbytes() could have just 
been a simple two line addition.  None of what is being proposed is necessary 
-- there are no known user problems being solved.  At best, this is unnecessary 
code churn.  At worse, it will be a breaking change.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

STINNER Victor  added the comment:

> It may be time to start emitting a warning if a Random subclass overrides 
> random() but not getrandbits() or vice versa.

Does someone know if there are users outside the stdlib of random subclass 
which only implements random()? I guess that a deprecation warning should help 
us to know :-)

There is also an implementation detail: technically, it's also sems to possible 
to only implement _randbelow(): see __init_subclass__(). But I'm not sure what 
happens in Python 3.8 if you implement _randbelow() but not random() not 
getrandbits().

In my experience, all RNG either generates random bits or random bytes, but not 
directly random floats. Usually, floats are computed from the other operations: 
that's what I'm proposing in BaseRandom. random() is now computed from 
getrandbits(). But it remains possible to override random(), as done in 
random.Random.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

It may be time to start emitting a warning if a Random subclass overrides 
random() but not getrandbits() or vice versa.

If you only override random(), methods which rely on generating random integers 
will use worse algorithm (slightly less uniform). If you only override 
getrandbits(), -- it is even worse, methods which rely on generating random 
floating point number will be not affected.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

STINNER Victor  added the comment:

Example with Python 3.8:
---
import random

class MyRandom(random.Random):
def getrandbits(self, n):
return 0

my = MyRandom()
print([my.randint(1, 6) for _ in range(3)])
print([my.random() for _ in range(3)])
---

Output:
---
[1, 1, 1]
[0.5654641798814677, 0.610057019404943, 0.7526620665660224]
---

[1, 1, 1] is what I expect, but what are these random [0.5654641798814677, 
0.610057019404943, 0.7526620665660224] numbers?

This behavior is surprising me. I would expect the base class to be "empty", 
not to inherit Mersenne Twister. If I don't implement all required abstract 
methods: I would either expect an error when the class is created, or at least 
when the method is called.


Raymond:
> Am not sure I'm comfortable with you defining random() in pure python 
> dividing by BPF -- that seems like a C level decision.

My PR 19631 doesn't change random.Random.random() (still implemented in C) nor 
random.SystemRandom.random() (same Python implementation): they should produce 
exactly the same random floating point numbers.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[Raymond Hettinger]

Raymond Hettinger  added the comment:

Having SystemRandom() use less memory is nice.  The seed() logic is reusable 
(not MT specific) and should be kept.  Historically, subclassers were supposed 
to supply random(), and the getrandbits() method was optional and just added 
greater range to randrange.  Am not sure I'm comfortable with you defining 
random() in pure python dividing by BPF -- that seems like a C level decision. 
Perhaps some factoring may be useful, but I worry that you're changing the user 
contracts in subtle ways and that we aren't really gaining anything out of 
this.  I'll just wait to see what everyone else has to say.  For me, I really 
wish you wouldn't do this and would have instead just added randbytes() in a 
way that was in harmony with the rest of the module.  I am getting afraid to 
submit bug reports because instead of fixing them, it triggers broad redesign 
and churn.  This all started because Antoine and Mark wanted getrandbits(0) to 
return 0; that was such a minor thing, and now the code is experien
 cing instability and is in some places measurably slower.

> The base class should implement randbytes() using getrandbits()

That is a reimagining of the API at odds with the 20+ year history of the 
module.  If we were starting out from scratch, that might have been a 
reasonable path, but with long standing, stable code, it doesn't make as much 
sense.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

STINNER Victor  added the comment:

Attached PR 19631 adds random.BaseRandom. random.SystemRandom now inherits from 
BaseRandom and so no longer inherits from _random.Random: an instance now only 
takes 48 bytes of memory, rather than 2568 bytes (on x86-64).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +18958
pull_request: https://github.com/python/cpython/pull/19631

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[Raymond Hettinger]

Raymond Hettinger  added the comment:

The randbytes() method could have been added effortlessly as a one line pure 
python method.  It only became complicated as a result of premature 
optimization into C code and as a result of ignoring the API promises.

You really don't have to redesign the whole module.  We have no user 
complaints.  There is no problem to be fixed.  IMO, this would be unnecessary 
churn.

--
nosy: +mark.dickinson, rhettinger, tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests:  -18957

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[Antoine Pitrou]

Change by Antoine Pitrou :


--
keywords: +patch
nosy: +pitrou
nosy_count: 1.0 -> 2.0
pull_requests: +18957
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/19539

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40346] Redesign random.Random class inheritance

[STINNER Victor]

Change by STINNER Victor :


--
title: Redesign random class inheritance -> Redesign random.Random class 
inheritance

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com