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

First-class function type #5287

Merged
merged 30 commits into from
Mar 27, 2020
Merged

First-class function type #5287

merged 30 commits into from
Mar 27, 2020

Conversation

pearu
Copy link
Contributor

@pearu pearu commented Feb 20, 2020

Closes #3405

This PR (as a continuation of PR #4967) implements:

  • Major revision on how Dispatcher and FunctionType instances interact.
  • Full support for jit-decorated functions as first-class type functions
    • The jit-decorated functions are considered first-class type functions when using firstclass=True option. For instance:
      @numba.njit
      def a(x):  # `a` is first-class type function, that is,
                 # it can be used as an object in other jit-decorated functions
          return x + 1
      
      @numba.jit()
      def foo(f, x):
          return f(x)
      
      assert foo(a, 3) == 4
      

@pearu
Copy link
Contributor Author

pearu commented Feb 20, 2020

@stuartarchibald to reproduce the issue of constant functions:

  1. uncomment the line in https://github.com/Quansight/numba/blob/6e190ed13987c2c33d065eebc807879e3307a9a0/numba/core/types/function.py#L116
  2. run the test_constant_functions test, for instance:
    pytest -sv numba/tests/test_function_type.py -k test_constant_functions
    
    that should print out the LLVM IR and fail with AssertionError: 246 != 579

@sklam
Copy link
Member

sklam commented Feb 24, 2020

It needs to handle njit functions that are generators, or reject gracefully:

from numba import njit

@njit(firstclass=True)
def gen(xs):
    for x in xs:
        x += 1
        yield x

@njit
def con(gen_fn, xs):
    return  [it for it in gen_fn(xs)]


con(gen, (1, 2, 3))

currently, it fails with:

numba/core/pythonapi.py in serialize_object(self, obj)
   1327         try:
-> 1328             gv = self.module.__serialized[obj]
   1329         except KeyError:

KeyError: (UniTuple(int64 x 3),) -> int64 generator(func=<function gen at 0x116defb90>, args=(UniTuple(int64 x 3),), has_finalizer=True)

@pearu
Copy link
Contributor Author

pearu commented Feb 25, 2020

In nopython mode, generators cannot be used as first-class function types because of the corresponding cfunc wrapper functions cannot hold the state of generators. So, raising UnsupportedError when njit(firstclass=True)-decorating generators.

In python mode, however, generators can be used as first-class functions.

@pearu pearu changed the title First-class function type [WIP] First-class function type Feb 25, 2020
@pearu
Copy link
Contributor Author

pearu commented Mar 2, 2020

To eliminate the need for firstclass=True, the notes in https://github.com/xnd-project/rbc/wiki/How-numba-resolves-function-calls%3F#numba-jit-decorated-functions-as-first-class-functions must be addressed.

Also, the implementation of FunctionType for jit-decorated functions needs to be made immutable: currently, the hash value depends on the last specialization of jit-decorated function.

@pearu pearu changed the title First-class function type First-class function type [WIP] Mar 3, 2020
@stuartarchibald stuartarchibald added this to the Numba 0.49 RC milestone Mar 9, 2020
numba/core/function_type.py Outdated Show resolved Hide resolved
numba/core/function_type.py Show resolved Hide resolved
numba/core/function_type.py Show resolved Hide resolved
@pearu
Copy link
Contributor Author

pearu commented Mar 24, 2020

@sklam I have addressed all your comments and feedback. Please review.

@pearu pearu requested a review from sklam March 24, 2020 20:00
@stuartarchibald
Copy link
Contributor

@sklam some notes from our discussion.

  • Need to move import from numba/__init__.py to somewhere more appropriate.
  • Need to move any reasonable candidates to numba/experimental/.
  • Issue warning of experimental feature if used (i.e. unification of function types triggers warning), perhaps a NumbaExperimentalFeatureWarning or similar.
  • Longer term goal: compilation should derive from more in depth static analysis.

Merge conditions:

  • @pearu are you able to commit to providing support for fixing parts of this that may need alterations and also to respond to user bugs? Thanks.

@pearu
Copy link
Contributor Author

pearu commented Mar 26, 2020

@stuartarchibald @sklam , yes, I would be happy to continue improving the feature and fix any related bugs.

@stuartarchibald
Copy link
Contributor

Thanks @pearu, that's great!

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.

I've reviewed the user docs.

docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
docs/source/reference/types.rst Outdated Show resolved Hide resolved
@stuartarchibald
Copy link
Contributor

Fixes to the docs in 984a24b look good, thanks @pearu .

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Mar 27, 2020
@sklam
Copy link
Member

sklam commented Mar 27, 2020

#5441 should be merged immediately after merging this

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.

Feature Request: Function as first-class type
3 participants