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

Changes to literal behaviour between <=0.49.0 and >=0.50.0 #6274

Closed
2 tasks done
JSKenyon opened this issue Sep 23, 2020 · 6 comments · Fixed by #6342
Closed
2 tasks done

Changes to literal behaviour between <=0.49.0 and >=0.50.0 #6274

JSKenyon opened this issue Sep 23, 2020 · 6 comments · Fixed by #6342
Labels
question Notes an issue as a question

Comments

@JSKenyon
Copy link

Reporting a bug

Reproducer

from numba.extending import overload, register_jitable
from numba import jit, literally


@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True)
def f(mode):
    return _f(literally(mode))


def _f(mode):
    return


@overload(_f, inline='always')
def _f_impl(mode):

    if mode.literal_value == "a":
        return impl_a
    elif mode.literal_value == "b":
        return impl_b
    else:
        raise ValueError("Unsupported mode!")


@register_jitable
def impl_a(mode):
    print("Using impl_a!")
    return sub_func(literally(mode))


@register_jitable
def impl_b(mode):
    print("Using impl_b!")
    return sub_func(literally(mode))


def sub_func(mode):
    return


@overload(sub_func, inline='always')
def _sub_func(mode):
    if mode.literal_value == "a":
        return lambda mode: 0
    else:
        return lambda mode: 1


if __name__ == "__main__":

    print(f("a"))
    print(f("b"))

Problem

The above fails when using numba>=0.50.0 with 'NoneType' object is not callable. This example runs without issue with numba<=0.49.0. I believe the issue stems from literally'ing an already literally'ed value - removing the literally calls from the sub_func invocations makes this run with numba>=0.50.0.

Question

Will this be the expected behaviour going forward and is the above pattern acceptable?

@bennahugo
Copy link

I think my situation is slightly different to your case @JSKenyon. I have a parallel numba and a serial numba implementation each using nested literals to further expand policies of the function. Those functions are both @jit functions. They are in turn called by a parent @jit function dependent on a branch evaluation. Moving all literally(...) to the parent @jit function has created a workaround for the NoneType raised by the compiler discussed in ratt-ru/codex-africanus#219.

However, I find the reason for the error not obvious. None of my literal policy types declare another literally(...) call but instead passes down a literal argument for nested literal evaluation.

bennahugo added a commit to ratt-ru/codex-africanus that referenced this issue Sep 23, 2020
@bennahugo
Copy link

bennahugo commented Sep 23, 2020

To expand on this here is an exact reproducer of the case I mentioned @JSKenyon

from numba.extending import overload, register_jitable
from numba import jit, literally


### WORKS in version 0.51.2 and in <=0.49                       

@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True)
def f(mode_1, mode_2):
    return subf(literally(mode_1), literally(mode_2))

@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True,
     inline="always")
def subf(mode_1,mode_2):
    return _f(mode_1, mode_2)

def _f(mode_1, mode_2):
    return  


#### DOES NOT WORK in version >=0.5, works in <=0.49
#
#@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True)
#def f(mode_1, mode_2):
#    return subf(literally(mode_1), mode_2)
#
#@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True,
#     inline="always")
#def subf(mode_1,mode_2):
#    return _f(mode_1, literally(mode_2))
#
#def _f(mode_1, mode_2):
#    return

##########################################
### Fake policies for nested static polymorphism
#########################################

@overload(_f, inline='always')
def _f_impl(mode_1, mode_2):

    if mode_1.literal_value == "a":
        return impl_a
    elif mode_1.literal_value == "b":
        return impl_b
    else:
        raise ValueError("Unsupported mode!")


@register_jitable
def impl_a(mode_1, mode_2):
    print("Using impl_a!")
    return sub_func_a(mode_2)


@register_jitable
def impl_b(mode_1, mode_2):
    print("Using impl_b!")
    return sub_func_b(mode_2)


def sub_func_a(mode_2):
    return


@overload(sub_func_a, inline='always')
def _sub_func_a(mode_2):
    if mode_2.literal_value == "c":
        return lambda mode_2: 0
    else:
        return lambda mode_2: 1


def sub_func_b(mode_2):
    return


@overload(sub_func_b, inline='always')
def _sub_func_b(mode_2):
    if mode_2.literal_value == "c":
        return lambda mode_2: 3
    else:
        return lambda mode_2: 4


if __name__ == "__main__":

    print(f("a", "c"))
    print(f("a", "d"))
    print(f("b", "c"))
    print(f("b", "d"))

@JSKenyon
Copy link
Author

To any Numba devs who get involved - this looks to be two separate but related issues.

@esc esc added the needtriage label Sep 23, 2020
@stuartarchibald
Copy link
Contributor

Thanks for the report. I think the behavioural change is covered by point 2 of the core feature changes for 0.51: https://numba.readthedocs.io/en/stable/release-notes.html#version-0-51-0-august-12-2020 namely:

Numba has internally switched to prefer non-literal types over literal ones so as to reduce function over-specialisation, this with view of speeding up compile times

I think that literally appearing to have an effect at a call site was an accident opposed to design, there was another ticket about this recently as the docs contain a mistake in relation to this. IIRC literally should be used in the callee to request re-dispatching the type inference mechanism with literal types where possible. For example, this sort of thing would achieve what is required under all versions of Numba supporting literally.

from numba.extending import overload, register_jitable
from numba import jit, literally, types


@jit(nopython=True, fastmath=True, parallel=False, cache=True, nogil=True)
def f(mode):
    return _f(mode)


def _f(mode):
    return


@overload(_f, inline='always')
def _f_impl(mode):

    # For the getattr to work, `mode` must be literal, so request it
    if not isinstance(mode, types.Literal):
        return lambda mode: literally(mode)

    if mode.literal_value == "a":
        return impl_a
    elif mode.literal_value == "b":
        return impl_b
    else:
        raise ValueError("Unsupported mode!")


@register_jitable
def impl_a(mode):
    print("Using impl_a!")
    return sub_func(mode)


@register_jitable
def impl_b(mode):
    print("Using impl_b!")
    return sub_func(mode)


def sub_func(mode):
    return


@overload(sub_func, inline='always')
def _sub_func(mode):

    # For the getattr to work, `mode` must be literal, so request it
    if not isinstance(mode, types.Literal):
        return lambda mode: literally(mode)

    if mode.literal_value == "a":
        return lambda mode: 0
    else:
        return lambda mode: 1


if __name__ == "__main__":

    print(f("a"))
    print(f("b"))

@JSKenyon
Copy link
Author

Thanks @stuartarchibald - this does seem more reliable! Will play around with it. :-)

@stuartarchibald
Copy link
Contributor

Thanks @stuartarchibald - this does seem more reliable! Will play around with it. :-)

No problem, the way literally is implemented in the compiler is "complicated" and could well have unknown issues. If you or @bennahugo discover more issues after making changes like in the above please shout (given the code similarity I presume the issues arose from the same code base, if not then I'll debug the other report too). Thanks!

@stuartarchibald stuartarchibald added question Notes an issue as a question and removed needtriage labels Sep 28, 2020
stuartarchibald added a commit to stuartarchibald/numba that referenced this issue Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Notes an issue as a question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants