Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typed.Dict and typed.List crashing on parametrized types #6402

Merged
merged 7 commits into from
Feb 2, 2021

Conversation

asodeur
Copy link
Contributor

@asodeur asodeur commented Oct 21, 2020

Fixes #6401 as described here

@gmarkall
Copy link
Member

Thanks for the PR! Once it's no longer [WIP], we can queue it up for review.

@stuartarchibald stuartarchibald added this to the PR Backlog milestone Oct 21, 2020
@asodeur asodeur changed the title [WIP] Fix typed.Dict and typed.List crashing on parametrized types Fix typed.Dict and typed.List crashing on parametrized types Oct 22, 2020
@gmarkall
Copy link
Member

Thanks very much for the PR, I've queued it for review.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch and for debugging and fixing this! The suggested fix seems to work around the issue described. I've reviewed the changes, there's just a few things to take a look at, most of which are in relation to the way Numba does things opposed to anything fundamental. Thanks again!

numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
Comment on lines 252 to 260
def run_test_parametrized_types_in_subprocess():
env = os.environ.copy()
subproc = subprocess.Popen(
[sys.executable, '-c',
'import numba.tests.test_dictimpl as test_mod\n' +
'test_mod.dict_vs_cache_vs_parametrized_worker()'],
env=env)
subproc.wait()
return subproc.returncode
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand why this is in a subprocess, is it with view of isolating a potential segfault? If the PR is fixing something that has a tendency to segfault, I'd rather it segfaults the test suite as a demonstration that it's not working than survive via isolation and report it, else we'd end up with all tests in subprocesses just-in-case?

Copy link
Contributor Author

@asodeur asodeur Dec 7, 2020

Choose a reason for hiding this comment

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

I have not found a reproducer w/o subprocess, yet. I've tried Dispatcher._make_finalizer(()(); Dispatcher._reset_overloads(); jit(...) to force re-loading from cache (which seems to do the job) but it still does not crash unless the cached function is used in another process. Is there anything more I can try and reset/delete in a single process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a little more experimentation I came-up with the below. Does the way I am resetting the dispatchers and the target context look resonable?

    def test_parametrized_types(self):
        """https://github.com/numba/numba/issues/6401"""

        register_model(ParametrizedType)(UniTupleModel)

        @typeof_impl.register(Parametrized)
        def typeof_unit(val, c):
            return ParametrizedType(val)

        @unbox(ParametrizedType)
        def unbox_parametrized(typ, obj, context):
            return context.unbox(types.UniTuple(typ.dtype, len(typ)), obj)

        @generated_jit
        def dict_vs_cache_vs_parametrized(v):
            typ = v

            def objmode_vs_cache_vs_parametrized_impl(v):
                # typed.List shows same behaviour after fix for #6397
                d = typed.Dict.empty(types.unicode_type, typ)
                d['data'] = v

            return objmode_vs_cache_vs_parametrized_impl

        @jit(nopython=True, cache=True)
        def set_parametrized_data(x, y):
            # Has had a tendency to segfault when the compiled function
            # was loaded from cache in a different process than the one
            # it was originally compiled in.
            # The new process is simulated below by resetting the dispatchers
            # and the target context
            dict_vs_cache_vs_parametrized(x)
            dict_vs_cache_vs_parametrized(y)

        x, y = Parametrized(('a', 'b')), Parametrized(('a',))
        set_parametrized_data(x, y)

        # reset dispatchers and targetctx to force re-load from cache as a new process would jit
        set_parametrized_data._make_finalizer()()
        set_parametrized_data._reset_overloads()
        set_parametrized_data.targetctx.init()

        for ii in range(50):  # <- somtimes works a few times
            assert set_parametrized_data(x, y) is None

numba/typed/typedobjectutils.py Outdated Show resolved Hide resolved
numba/typed/typedobjectutils.py Outdated Show resolved Hide resolved
numba/typed/typedobjectutils.py Outdated Show resolved Hide resolved
numba/typed/typedobjectutils.py Outdated Show resolved Hide resolved
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Nov 27, 2020
asodeur and others added 2 commits December 1, 2020 13:49
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 1, 2021
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
Co-authored-by: Siu Kwan Lam <1929845+sklam@users.noreply.github.com>
@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Feb 2, 2021
@sklam sklam merged commit 862bd15 into numba:master Feb 2, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash involving typed.Dict, caching, and parametrized type
4 participants