Remove leftover Python 2 code and the mklrand_py_helper.h shim by vlad-perevezentsev · Pull Request #192 · IntelPython/mkl_random

vlad-perevezentsev · 2026-10-15T11:53:27Z

The project requires Python 3.10+, so the mklrand_py_helper.h helpers only wrapped calls that Cython already declares in cpython.bytes; the header is gone and mklrand.pyx call PyBytes_* directly, which also drops its reliance on include order.
The remaining Python 2 leftovers go too: the no-op __future__ import, the str return type in the bytes docstring, and sys.maxint in the tomaxint example.

PyBytes_Check is used instead of isinstance(x, bytes) because the module-level bytes = _rand.bytes alias shadows the builtin here. No behavior change.

antonwolfy

Looks good to me with small nits below.

Possible follow-ups outside this PR, for the same Python 2 cleanup:

mkl_random/__init__.py:26: from __future__ import absolute_import, division, print_function does nothing on Python 3.
mkl_random/mklrand.pyx:2664-2670: the bytes() docstring says out : str and shows a Python 2 string repr; it should be bytes with a b'...' example.
mkl_random/mklrand.pyx:3581-3583: the tomaxint example uses sys.maxint, which doesn't exist in Python 3 (sys.maxsize).

With the Python 2 branches gone, the three helpers only wrapped calls that Cython already declares in cpython.bytes, so the header had no reason to exist. Dropping it also removes its hidden dependency on include order: it used npy_intp while including only <Python.h>. PyBytes_Check stands in for isinstance(x, bytes) because the module-level `bytes = _rand.bytes` alias shadows the builtin here.

absolute_import, division and print_function are unconditional on Python 3, and the project requires Python 3.10+.

`bytes` documented its return as `str` and showed a repr without the `b` prefix; `tomaxint` compared against `sys.maxint`. The comparison now uses `np.iinfo("long").max`, matching the range in the summary line, and the bool array drops the `dtype=bool` suffix NumPy 2 no longer prints.

Per review, "Removed" is for functionality users lose. Log the cleanup under "Changed" and the docstring corrections under "Fixed".

The return type was corrected to `bytes`, but the line below it still read "String of length `length`.", matching NumPy's own wording.

jharlow-intel

Thanks, nice cleanup! The swap to the cpython.bytes cimports is one-to-one, nothing still references the removed header, and the existing get_state/set_state and bytes() tests cover the touched paths. Thanks for picking up Anton's follow-ups too.

antonwolfy

A few Python 2 leftovers remain outside the diff, so either the description's "the remaining Python 2 leftovers go too" needs narrowing, or these could go in as well:

mklrand.pyx:2096 and :3148: <type 'float'> / <type 'int'> in the examples
mklrand.pyx:3103: "the C long type used by Python 2"
mklrand.pyx:2555: 'np.int' as the randint default (removed in NumPy 1.24)
src/mkl_distributions.cpp:1510,1513: sys.maxint in the Zipf comment

Out of scope, but worth an issue: np.long at mklrand.pyx:2829, :2844 and :7101 doesn't exist in NumPy 1.26, which numpy >=1.26.4 still allows, so choice(..., p=...) and permutation(int) raise AttributeError there. CI starts at NumPy 2.2, so it isn't caught.

Nit: "mklrand.pyx call" in the description should be "calls".

The `random_sample` and `random_integers` examples printed `<type '...'>` reprs, `random_integers` described `np.int` as Python 2's "short" integer type, `randint` documented its default dtype as `'np.int'` (removed in NumPy 1.24), and the `zipf` comment described the C long bounds as `sys.maxint`. Wording follows NumPy's current docstrings. Also marks the `tomaxint` example output as `# random`, which the removed `sys.maxint` lines used to imply.

If irk_get_stream_size returns 0 or a negative MKL error code on the first pass, the loop takes the save branch with bytesPtr never assigned, which g++ reports under -Wmaybe-uninitialized. NULL also makes that path well defined: vslSaveStreamM rejects a NULL buffer instead of writing.

The single entry read as if both docstrings had both defects; only `bytes` documented a `str` return and only `tomaxint` used `sys.maxint`.

Same -Wmaybe-uninitialized case as the buffer pointer: `err` is assigned inside the lock and read after it, so g++ cannot prove the assignment. Zero is the "no error" value the checks below compare against.

antonwolfy

Thanks for the updates. Two follow-ups on the new random_integers docstring text below. I also reopened the set_state thread, since the length check is still missing.

The np.long uses at mklrand.pyx:2829, :2844 and :7102 (broken on NumPy 1.26) aren't tracked anywhere yet either; worth an issue together with the set_state one.

Per review: on NumPy 2 `numpy.int_` is `intp`, which is 64-bit on Windows, while this returns a 32-bit C long there, so the wording borrowed from NumPy is wrong. The example type was wrong too, in the same way the original `<type 'int'>` was: `random_integers` delegates to `randint(..., dtype="l")`, which returns a NumPy scalar, not a Python int. Also fixes the neighbouring example, which passed a float `size` that raises TypeError now, and marks the random output as NumPy does.

antonwolfy

Thank you @vlad-perevezentsev

…lbacks

vlad-perevezentsev added this to the 1.6.0 release milestone Oct 15, 2026

vlad-perevezentsev self-assigned this Oct 15, 2026

vlad-perevezentsev requested review from antonwolfy, jharlow-intel, ndgrigorian and xaleryb as code owners October 15, 2026 11:53

vlad-perevezentsev added 2 commits October 15, 2026 04:53

Remove dead PyString fallbacks from mklrand_py_helper.h

03b8f8c

Add gh-192 to changelog

07774f9

antonwolfy reviewed Oct 15, 2026

View reviewed changes

Comment thread mkl_random/src/mklrand_py_helper.h Outdated

Comment thread mkl_random/src/mklrand_py_helper.h Outdated

Comment thread CHANGELOG.md Outdated

vlad-perevezentsev added 2 commits October 15, 2026 06:50

Remove the no-op __future__ import

289076f

absolute_import, division and print_function are unconditional on Python 3, and the project requires Python 3.10+.

vlad-perevezentsev changed the title ~~Remove dead PyString fallbacks from mklrand_py_helper.h~~ Remove leftover Python 2 code and the mklrand_py_helper.h shim Oct 15, 2026

vlad-perevezentsev added 3 commits October 15, 2026 06:51

Move the gh-192 changelog entry out of Removed

f77982f

Per review, "Removed" is for functionality users lose. Log the cleanup under "Changed" and the docstring corrections under "Fixed".

Describe the bytes return value as bytes, not a string

5285bf4

The return type was corrected to `bytes`, but the line below it still read "String of length `length`.", matching NumPy's own wording.

jharlow-intel approved these changes Oct 15, 2026

View reviewed changes

jharlow-intel mentioned this pull request Oct 15, 2026

Document MKLRandomState in the API reference and align docstrings with NumPy #165

Open

antonwolfy reviewed Oct 15, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread mkl_random/mklrand.pyx Outdated

Comment thread mkl_random/mklrand.pyx

Comment thread mkl_random/mklrand.pyx

vlad-perevezentsev added 4 commits October 15, 2026 16:22

Split the gh-192 changelog entries

da0d22c

The single entry read as if both docstrings had both defects; only `bytes` documented a `str` return and only `tomaxint` used `sys.maxint`.

Initialize the set_state error code to zero

543277e

Same -Wmaybe-uninitialized case as the buffer pointer: `err` is assigned inside the lock and read after it, so g++ cannot prove the assignment. Zero is the "no error" value the checks below compare against.

antonwolfy reviewed Oct 16, 2026

View reviewed changes

Comment thread mkl_random/mklrand.pyx Outdated

Comment thread mkl_random/mklrand.pyx Outdated

vlad-perevezentsev added 2 commits October 16, 2026 02:45

Add the random_integers docstring fix to the changelog

336a8f1

This was referenced Oct 16, 2026

set_state does not validate the state buffer length, causing out-of-bounds reads #193

Open

np.long breaks choice(p=...) and permutation(int) on NumPy 1.26 #194

Open

antonwolfy approved these changes Oct 16, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into remove-pystring-fal…

198679e

…lbacks

vlad-perevezentsev merged commit 5eaa069 into master Oct 16, 2026
172 checks passed

Navigation Menu

Repository navigation

Remove leftover Python 2 code and the mklrand_py_helper.h shim - #192

Remove leftover Python 2 code and the mklrand_py_helper.h shim#192
vlad-perevezentsev merged 14 commits into
masterfrom
remove-pystring-fallbacks

vlad-perevezentsev commented Oct 15, 2026 •

edited

Loading

Uh oh!

antonwolfy left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jharlow-intel left a comment

Uh oh!

antonwolfy left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antonwolfy left a comment

Uh oh!

Uh oh!

Uh oh!

antonwolfy left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

vlad-perevezentsev commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonwolfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jharlow-intel left a comment

Choose a reason for hiding this comment

Uh oh!

antonwolfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antonwolfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vlad-perevezentsev commented Oct 15, 2026 •

edited

Loading

antonwolfy left a comment •

edited

Loading

antonwolfy left a comment •

edited

Loading