Skip to content

Support for Numpy BitGenerators PR#1 - Core Generator Support - #8031

Merged
sklam merged 22 commits into
numba:mainfrom
kc611:np_generator_core
Jun 17, 2022
Merged

sklam merged 22 commits into
numba:mainfrom
kc611:np_generator_core

Conversation

@kc611

@kc611 kc611 commented May 10, 2022 •

Copy link
Copy Markdown
Contributor

This PR adds the core BitGenerator implementation from #7900 and adds support for np.random.Generator().random()

cc @esc @stuartarchibald

@kc611 kc611 changed the title Added Support for Numpy BitGenerators Added Support for Numpy BitGenerators PR#1 - Core Generator Support May 10, 2022
@kc611

kc611 commented May 10, 2022

Copy link
Copy Markdown
Contributor Author

I just checked the CI logs, it doesn't look like the tests I added are actually being run in the pipeline (probably because of them being in a new file). Where are the configuration files located for registering that ?

@esc

esc commented May 11, 2022

Copy link
Copy Markdown
Member

I just checked the CI logs, it doesn't look like the tests I added are actually being run in the pipeline (probably because of them being in a new file). Where are the configuration files located for registering that ?

You will need to make the test class inherit from TestCase so as to mark it as a test and such that it is picked up by the testing framework. I created a temporary PR at:

#8033

Which does that and also deliberately makes the test fail to provide an easy way to tell if it did indeed fail. You can probably just grab the following diff to make it work:

diff --git a/numba/tests/test_np_randomgen.py b/numba/tests/test_np_randomgen.py
index f0830b10c2..9d33d62914 100644
--- a/numba/tests/test_np_randomgen.py
+++ b/numba/tests/test_np_randomgen.py
@@ -1,10 +1,12 @@
 import numpy as np
 import numba
+from numba.tests.support import TestCase

 from numpy.random import MT19937, Generator


-class TestRandomGenerators:
+class TestRandomGenerators(TestCase):
+
     def check_numpy_parity(self, distribution_func,
                            bitgen_instance=None, seed=1):
         distribution_func = numba.njit(distribution_func)

@kc611
kc611 force-pushed the np_generator_core branch from af4cd2d to 273e3a7 Compare May 11, 2022 11:34
@kc611

kc611 commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

What kind of documentation should we add for this PR ?

@kc611 kc611 changed the title Added Support for Numpy BitGenerators PR#1 - Core Generator Support Support for Numpy BitGenerators PR#1 - Core Generator Support May 11, 2022

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I left some comments for your consideration.

Comment thread numba/core/typing/typeof.py
Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/np/random/generator_methods.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
@esc

esc commented May 12, 2022

Copy link
Copy Markdown
Member

What kind of documentation should we add for this PR ?

I would argue that the section in: https://numba.readthedocs.io/en/stable/reference/numpysupported.html#modules

will need an overhaul to describe how the NumPy random number generator state can now traverse the JIT boundary.

@esc

esc commented May 16, 2022

Copy link
Copy Markdown
Member

@kc611 thank you for the updates. One immediate thing to note is that the doc build seems to be failing. This is because for the public CI in Numba we have elected to treat warnings as errors and so the build fails. The error is

/home/vsts/work/1/s/docs/source/reference/numpysupported.rst:586:Title underline too short.

which ought to be quite straightforward to fix.

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc needs some changes for it to render correctly.

Comment thread docs/source/reference/numpysupported.rst
@kc611
kc611 requested a review from esc May 16, 2022 11:11

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three minor documentation nitpicks.

Comment thread docs/source/reference/numpysupported.rst Outdated
Comment thread numba/core/typing/typeof.py Outdated
Comment thread numba/np/random/generator_methods.py Outdated

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @kc611, great to see it implemented. Having written a some of the code for this I've done a limited review to comment on general design, docs and ensuring correctness. Outstanding issues for discussion include:

  • object life times: what happens if a bit generator instance is deleted whilst in use by an njit function?
  • caching @njit(cache=True): what happens to those pointers when the code is lowered, think they end up as runtime specific ints, cache replay will likely cause segfaults!
  • Is there a good way to test the bit generator sequences so as to separate their correctness from the correctness of derived properties such as the random distributions?

Thanks again!

Comment thread docs/source/reference/numpysupported.rst Outdated
Comment thread numba/core/boxing.py Outdated
Comment thread numba/np/random/generator_methods.py Outdated
Comment thread numba/np/random/generator_methods.py
Comment thread numba/np/random/generator_methods.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/np/random/generator_methods.py
Comment thread numba/core/boxing.py
Comment thread numba/core/boxing.py Outdated
@stuartarchibald

Copy link
Copy Markdown
Contributor

I think this might summarise the current issue observed with reference counting for the Generator type. This shows what happens with a NumPy array and what happens with a custom type that does something similar to that required in the case of the Generator type. It might be easier debugging with this than with a full implementation?

from contextlib import ContextDecorator
import gc
import sys
import numpy as np

from numba import njit, types, objmode
from numba.extending import typeof_impl, models, register_model
from numba.extending import box, unbox, NativeValue
from numba.core import cgutils
from numba.core.runtime import rtsys


class Thing(object):
    def __init__(self):
        pass


class ThingType(types.Type):
    def __init__(self):
        super(ThingType, self).__init__(name='Thing')


thing_type = ThingType()


@typeof_impl.register(Thing)
def typeof_thing(val, c):
    return thing_type


@register_model(ThingType)
class ThingModel(models.StructModel):
    def __init__(self, dmm, fe_type):
        members = [
            ('meminfo', types.MemInfoPointer(types.voidptr)),
            ('parent', types.pyobject),
        ]
        models.StructModel.__init__(self, dmm, fe_type, members)


@unbox(ThingType)
def unbox_thing(typ, obj, c):
    thing_struct = cgutils.create_struct_proxy(typ)(c.context, c.builder)
    thing_struct.parent = obj
    NULL = cgutils.voidptr_t(None)
    thing_struct.meminfo = c.pyapi.nrt_meminfo_new_from_pyobject(
        NULL,  # there's no data
        obj,   # the python object, the call to nrt_meminfo_new_from_pyobject
               # will py_incref
    )
    is_py_error = cgutils.is_not_null(c.builder, c.pyapi.err_occurred())
    return NativeValue(thing_struct._getvalue(), is_error=is_py_error)


@box(ThingType)
def box_thing(typ, val, c):
    thing_struct = cgutils.create_struct_proxy(typ)(c.context, c.builder,
                                                    value=val)
    res = thing_struct.parent
    c.context.nrt.decref(c.builder, typ, val)
    return res


class memory_leak_check(ContextDecorator):

    def __init__(self, the_obj):
        self._the_obj = the_obj

    def __enter__(self):
        # Clean up any NRT-backed objects hanging in a dead reference cycle
        gc.collect()
        print(f"Python refcount: {sys.getrefcount(self._the_obj)}")
        self.__init_stats = rtsys.get_allocation_stats()

    def __exit__(self, *exc):
        gc.collect()
        old = self.__init_stats
        new = rtsys.get_allocation_stats()
        total_alloc = new.alloc - old.alloc
        total_free = new.free - old.free
        total_mi_alloc = new.mi_alloc - old.mi_alloc
        total_mi_free = new.mi_free - old.mi_free
        print(f"Allocs: {total_alloc}. Frees: {total_free}")
        print(f"MI Allocs: {total_mi_alloc}. MI Frees: {total_mi_free}")
        gc.collect()
        print(f"Python refcount: {sys.getrefcount(self._the_obj)}")


def np_ty(): return np.ones(3)


for Clazz in (np_ty, Thing):
    print('\n', f"Testing Type: {Clazz}".center(80, '='), '\n')
    print('Testing no-op'.center(80, '_'))

    thing_inst1 = Clazz()
    print(f"Python refcount: {sys.getrefcount(thing_inst1)}")  # should be 2
    with memory_leak_check(thing_inst1):
        thing_inst1  # Python refcount should be 3, the ctxmanager holds 1 more

    @njit
    def foo(x):
        return x

    thing_inst2 = Clazz()

    print('Testing foo'.center(80, '_'))
    with memory_leak_check(thing_inst2):
        initial_id = id(thing_inst2)
        result = foo(thing_inst2)
        result_id = id(result)
        print(f"ID check: {result is thing_inst2}")

    @njit
    def bar(x):
        pass  # x unused

    print('Testing bar'.center(80, '_'))
    thing_inst3 = Clazz()
    with memory_leak_check(thing_inst3):
        bar(thing_inst3)

    @njit
    def baz(x):
        with objmode():
            print(f"In objmode block {sys.getrefcount(x)}")

    print('Testing baz'.center(80, '_'))
    thing_inst4 = Clazz()
    with memory_leak_check(thing_inst4):
        baz(thing_inst4)

@kc611

kc611 commented May 31, 2022 •

Copy link
Copy Markdown
Contributor Author

If we have a Python incref for the parent during boxing, the stats seems exactly the same. But does that cause further problems ?

(In the above example that would be equivalent to having c.pyapi.incref(res) during boxing)

@kc611
kc611 requested review from esc and stuartarchibald June 1, 2022 10:46
@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review labels Jun 1, 2022
Comment thread numba/tests/test_np_randomgen.py
@sklam sklam added this to the Numba 0.56 RC milestone Jun 1, 2022
@kc611
kc611 force-pushed the np_generator_core branch from 5dc906e to 40e87e9 Compare June 15, 2022 09:05
@esc esc added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Jun 15, 2022
@esc

esc commented Jun 15, 2022

Copy link
Copy Markdown
Member

Build numba_smoketest_cpu_yaml_105 has started

@esc

esc commented Jun 16, 2022

Copy link
Copy Markdown
Member

Build numba_smoketest_cpu_yaml_105 has started

This has now finally gone green 🟢 and I am marking this as RTM.

@esc esc added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 16, 2022
@sklam
sklam merged commit 2770647 into numba:main Jun 17, 2022
@esc

esc commented Jun 17, 2022

Copy link
Copy Markdown
Member

@kc611 thanks for the patch!

@pwuertz

pwuertz commented Aug 19, 2024

Copy link
Copy Markdown
Contributor

The documentation added in this PR states that

The current Numba support for :py:class:Generator is not thread-safe, hence we
do not recommend using :py:class:Generator methods in methods with parallel
execution logic.

Could you elaborate a bit on this? I understand that if I pass a Generator into a Numba function, Numba will take a reference to its BitGenerator for reading/writing its state - so naturally, one shouldn't use it concurrently from multiple threads. Are there any dangers beyond this type of misuse?

More specifically, is it safe to run multiple nogil=True Numba functions concurrently if each call receives a separate Generator, e.g. created by Generator.spawn()? Or does the Numba Generator implementation use some sort of global state management that prohibits any sort of parallelism?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed Effort - long Long size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants