task: add patch methods for mkl_random by jharlow-intel · Pull Request #90 · IntelPython/mkl_random

jharlow-intel · 2026-02-12T20:40:39Z

Gonna need some expert's eyes on this one, but it built fine and the tests look okay.

Definitely going to have to make sure it properly interfaced numpy.random though

Copilot

Pull request overview

This PR adds monkey-patching functionality to mkl_random, allowing users to temporarily or permanently replace numpy.random functions with their mkl_random equivalents. The implementation provides both imperative (monkey_patch(), restore()) and context manager (mkl_random.mkl_random()) interfaces.

Changes:

Adds new _patch.pyx Cython module implementing patching logic with thread-local state tracking
Extends setup.py to build the new _patch extension module
Exports patch functions (monkey_patch, use_in_numpy, restore, is_patched, patched_names, mkl_random) in __init__.py
Adds comprehensive test suite in test_patch.py covering basic patching, restoration, and context manager functionality

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.

File	Description
setup.py	Adds Extension configuration for mkl_random._patch module
mkl_random/src/_patch.pyx	Implements core patching logic with thread-local storage, patch/unpatch methods, and context manager
mkl_random/init.py	Exports patch-related functions to public API
mkl_random/tests/test_patch.py	Adds tests for patching, restoration, context manager, and patched function behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mkl_random/src/_patch.pyx

Copilot · 2026-02-20T15:17:26Z

setup.py

+        Extension(
+            "mkl_random._patch",
+            sources=[join("mkl_random", "src", "_patch.pyx")],
+            include_dirs=[np.get_include()],
+            define_macros=defs + [("NDEBUG", None)],
+            language="c",
        )


The _patch extension includes define_macros with PY_ARRAY_UNIQUE_SYMBOL, but _patch.pyx doesn't use the NumPy C API (no cimport numpy or usage of numpy C structures). While this doesn't cause harm, these macros are unnecessary for this extension and could be removed for clarity. The extension only needs include_dirs=[np.get_include()] to compile successfully with Cython's NumPy support.

@antonwolfy @ndgrigorian I'm unsure about this one honestly, looks like removing PY_ARRAY_UNIQUE_SYMBOL from setup.py is ultimately unnecessary

mkl_random/src/_patch.pyx

mkl_random/tests/test_patch.py

mkl_random/src/_patch.pyx

mkl_random/tests/test_patch.py

mkl_random/src/_patch.pyx

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

mkl_random/init.py:101

The patching API is imported into the top-level package, but the new public symbols (monkey_patch, use_in_numpy, restore, is_patched, patched_names, mkl_random) are not added to __all__. This makes the export surface inconsistent with the rest of this module (which explicitly enumerates public names) and can break from mkl_random import * expectations.

from ._patch import monkey_patch, use_in_numpy, restore, is_patched, patched_names, mkl_random
from mkl_random import interfaces

__all__ = [
    "MKLRandomState",
    "RandomState",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T12:56:51Z

mkl_random/src/_patch.pyx

+            if local_count <= 0:
+                if verbose:
+                    print(
+                        "Warning: restore called more times than monkey_patch in this thread."
+                    )


restore(verbose=True) currently emits a warning via print(...). Elsewhere in this project warnings are surfaced via warnings.warn(...) (often with an appropriate category/stacklevel). Using warnings.warn here would make the warning easier to filter/test and avoids writing directly to stdout.

Copilot · 2026-03-05T12:56:52Z

mkl_random/src/_patch.pyx

+def use_in_numpy(numpy_module=None, names=None, strict=False, verbose=False):
+    """
+    Backward-compatible alias for monkey_patch().
+    """
+    monkey_patch(
+        numpy_module=numpy_module,
+        names=names,
+        strict=strict,
+        verbose=verbose,
+    )


use_in_numpy() is documented as a backward-compatible alias for monkey_patch(), but it's currently a separate wrapper function. This breaks identity-based expectations (and the added test asserts use_in_numpy is monkey_patch). Either make use_in_numpy a true alias (assignment) or adjust the test and docs to reflect wrapper semantics.

Suggested change

def use_in_numpy(numpy_module=None, names=None, strict=False, verbose=False):

"""

Backward-compatible alias for monkey_patch().

"""

monkey_patch(

numpy_module=numpy_module,

names=names,

strict=strict,

verbose=verbose,

)

# Backward-compatible alias for monkey_patch().

use_in_numpy = monkey_patch

Copilot · 2026-03-05T12:56:52Z

mkl_random/tests/test_patch.py

+    mkl_random.monkey_patch(np)
+    mkl_random.monkey_patch(np)
+
+    assert mkl_random.is_patched()
+    assert np.random.normal is mkl_random.mklrand.normal
+
+    mkl_random.restore()
+    assert mkl_random.is_patched()
+    assert np.random.normal is mkl_random.mklrand.normal
+
+    mkl_random.restore()
+    assert not mkl_random.is_patched()
+    assert np.random.normal is orig_normal


test_patch_redundant_patching doesn't use a try/finally (or fixture finalizer) to guarantee mkl_random.restore() is called if an intermediate assertion fails. Because this test intentionally keeps NumPy patched after the first restore(), a failure before the final cleanup can leak global patched state into subsequent tests.

Suggested change

mkl_random.monkey_patch(np)

mkl_random.monkey_patch(np)

assert mkl_random.is_patched()

assert np.random.normal is mkl_random.mklrand.normal

mkl_random.restore()

assert mkl_random.is_patched()

assert np.random.normal is mkl_random.mklrand.normal

mkl_random.restore()

assert not mkl_random.is_patched()

assert np.random.normal is orig_normal

try:

mkl_random.monkey_patch(np)

mkl_random.monkey_patch(np)

assert mkl_random.is_patched()

assert np.random.normal is mkl_random.mklrand.normal

mkl_random.restore()

assert mkl_random.is_patched()

assert np.random.normal is mkl_random.mklrand.normal

mkl_random.restore()

assert not mkl_random.is_patched()

assert np.random.normal is orig_normal

finally:

# Ensure that any remaining patches are removed even if the test fails

while mkl_random.is_patched():

mkl_random.restore()

Copilot · 2026-03-05T12:56:52Z

mkl_random/src/_patch.pyx

+    # Common global sampling helpers
+    "random",
+    "random_sample",
+    "sample",
+    "rand",
+    "randn",
+    "bytes",


_DEFAULT_NAMES includes very common NumPy APIs like numpy.random.random and numpy.random.sample, but mkl_random.mklrand doesn't define random/sample (it has random_sample). With the current hasattr(_mr, name) check, those names will silently remain unpatched in non-strict mode, which undermines the goal of routing NumPy calls through mkl_random. Consider adding random/sample aliases on the mklrand side or explicitly mapping NumPy's random/sample to mklrand.random_sample during patching.

Copilot · 2026-03-05T12:56:53Z

mkl_random/src/_patch.pyx

+
+    def _normalize_names(self, names):
+        if names is None:
+            names = _DEFAULT_NAMES


_normalize_names() uses tuple(names), so passing a string like names="normal" will be treated as an iterable of characters and attempt to patch n, o, etc. This leads to confusing behavior and hard-to-diagnose failures. Consider rejecting str/bytes explicitly (and/or accepting a single name by wrapping it in a 1-tuple) and validating that all entries are strings.

Suggested change

names = _DEFAULT_NAMES

names = _DEFAULT_NAMES

elif isinstance(names, str):

# Treat a single string as a single name, not an iterable of characters

names = (names,)

elif isinstance(names, bytes):

# Attribute names must be strings; reject bytes explicitly

raise TypeError("names must be a string or an iterable of strings, not bytes")

else:

names = tuple(names)

for name in names:

if not isinstance(name, str):

raise TypeError("All names must be strings; got {!r} of type {!r}".format(name, type(name)))

ndgrigorian · 2026-03-05T16:12:24Z

mkl_random/src/_patch.pyx

@@ -0,0 +1,292 @@
+# Copyright (c) 2019, Intel Corporation


I don't think we need Cython here

like with mkl_fft's patch, we can do it entirely on the Python level. We aren't using any of NumPy/Python C API.

Oops, overlooked that thanks for mentioning it

task: add patch methods for mkl_random

f007ddd

jharlow-intel self-assigned this Feb 12, 2026

jharlow-intel requested review from antonwolfy, ndgrigorian and xaleryb as code owners February 12, 2026 20:40

jharlow-intel added the enhancement label Feb 12, 2026

jharlow-intel requested a review from Copilot February 20, 2026 15:10

Copilot started reviewing on behalf of jharlow-intel February 20, 2026 15:11 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

jharlow-intel changed the base branch from dev-milestone to master March 5, 2026 12:29

jharlow-intel and others added 3 commits March 5, 2026 05:30

Merge branch 'master' into task/patch-numpy

15404d2

fix: patching to match mkl_fft

c56e34b

fix: bot review

1f403ad

jharlow-intel requested a review from Copilot March 5, 2026 12:49

Copilot started reviewing on behalf of jharlow-intel March 5, 2026 12:50 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

jharlow-intel added 2 commits March 5, 2026 06:59

fix: precommit check

cf70971

fix: precommit check

4de0d12

ndgrigorian reviewed Mar 5, 2026

View reviewed changes

-            names = _DEFAULT_NAMES
+            names = _DEFAULT_NAMES
+        elif isinstance(names, str):
+            # Treat a single string as a single name, not an iterable of characters
+            names = (names,)
+        elif isinstance(names, bytes):
+            # Attribute names must be strings; reject bytes explicitly
+            raise TypeError("names must be a string or an iterable of strings, not bytes")
+        else:
+            names = tuple(names)
+        for name in names:
+            if not isinstance(name, str):
+                raise TypeError("All names must be strings; got {!r} of type {!r}".format(name, type(name)))

Conversation

jharlow-intel commented Feb 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

jharlow-intel Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

ndgrigorian Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

jharlow-intel Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants