py3: problems with tests that use random_element

[embray]

Lots and lots of tests fail on Python 3 due to the use of random_elements in tests.

The problem, I've found, is not to do with seeding the RNG. Python's random produces the same sequence with the same seed on Python 2 and 3. However, there is a slight difference in the implementation of random.randint, such that it does produce different values, annoyingly.

I'm not sure how we want to deal with this. In Sage there's a sage.misc.prandom that for some reason provides wrappers around functions from Python's random module. Perhaps one thing we could do is ensure that sage.misc.prandom is always used, and modify its wrapper to randint (actually randrange, which randint is implemented on top of), to provide consistent results.

Component: python3

Author: Erik Bray

Branch/Commit: 4ac044a

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24508

[embray]

comment:1

Also worth pointing out: There were good reasons to change the implementation, AFAICT, as some bugs were addressed. So using the Python 3 implementation might be better. However, that would mean fixing hundreds, possibly thousands of tests, so maybe it's not such a good idea (or maybe the Python 2 implementation should be used only in testing or something).

[fchapoton]

comment:2

there is also an issue about

src/sage/interfaces/interface.py: 
       return long(randstate().seed()&0x1FFFFFFF)

that I have already point out elsewhere

[embray]

comment:3

What's really the problem there? I think the long() can just be dropped.

[fchapoton]

comment:4

well, nobody tried to drop it (not me at least)

[embray]

comment:5

It's gone in my branch. I don't think it has anything to do with this problem.

[embray]

comment:6

Here's a possible workaround for this issue. This is far from ideal in that it wholesale copy/pastes the Python 2 implementation of random.Random. Hypothetically we could get away with copying only smaller sections, but this seemed like the most practical and sure to be consistent solution.

This is also not great since we should be testing against the real random.Random implementation being used at runtime. I think that for the sake of initial porting of Python 3 this is acceptable, however, and the likelihood of bugs specific to the Random implementation are low. This can be removed once Sage drops Python 2 support, whenever that happens...

---

New commits:

c3a0c8b	fix indentation--the entire randstate class body has an extra space of indentation
bc707b2	Update randstate.python_random to allow passing in an optional alternative to random.Random to use as the Python RNG.
d098e42	Provide a workaround for https://github.com/sagemath/sage/issues/24508

[embray]

Branch: u/embray/python3/ticket-24508

[embray]

Dependencies: #24787

[embray]

Author: Erik Bray

[embray]

Commit: d098e42

[embray]

comment:7

I confirmed that this fixed most tests that were known to be failing due to this issue (of which there were quite a lot).

[jdemeyer]

comment:8

1. Shouldn't you use _py2_random.py also on Python 2? Then you are sure that doctests will remain consistent, independently of any future changes to the upstream random module.

2. You can optimize if self._python_random is not None and type(self._python_random) is cls: to type(self._python_random) is cls:

[embray]

comment:9

Replying to @jdemeyer:

1. Shouldn't you use _py2_random.py also on Python 2? Then you are sure that doctests will remain consistent, independently of any future changes to the upstream random module.

I considered that--it seemed unlikely considering that through Python 2.7.14 this has never been an issue, and most non-essential fixes are not being backported to Python 2.

Then again, Python 2 will still be receiving security-related fixes for a while, and the random module is definitely a potential target for security fixes, so indeed it might be worth pinning even for Python 2.

2. You can optimize if self._python_random is not None and type(self._python_random) is cls: to type(self._python_random) is cls:

A minor point but true.

[sagetrac-git]

Changed commit from d098e42 to 4ac044a

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c9bf350	Small simplification
4ac044a	Use the baked-in copy of the random module even on Python 2

[embray]

New commits:

c9bf350	Small simplification
4ac044a	Use the baked-in copy of the random module even on Python 2

[jdemeyer]

Changed dependencies from #24787 to none

[jdemeyer]

Reviewer: Jeroen Demeyer

[embray]

comment:16

Patchbot suggests that "plugins failed" but it's completely non-obvious from the output what plugins failed...

[vbraun]

Changed branch from u/embray/python3/ticket-24508 to 4ac044a