Add facility to support with-contexts #3017

sklam · 2018-06-06T19:49:48Z

Adds pluggable facility to support with contexts. Two builtin contextmanagers are implemented for testing.

and some cleanup

# Conflicts: # numba/ir_utils.py

codecov-io · 2018-06-06T21:16:06Z

Codecov Report

Merging #3017 into master will increase coverage by 2.39%.
The diff coverage is 81%.

@@            Coverage Diff            @@
##           master   #3017      +/-   ##
=========================================
+ Coverage    78.7%   81.1%   +2.39%     
=========================================
  Files         384     386       +2     
  Lines       75091   75675     +584     
  Branches     8436    8488      +52     
=========================================
+ Hits        59098   61373    +2275     
+ Misses      14740   13003    -1737     
- Partials     1253    1299      +46

stuartarchibald

Thanks for the patch @sklam. Looks like a good start on this feature set. Only a few minor comments, I'll add more items directly to the ticket.

stuartarchibald · 2018-06-12T12:27:24Z

numba/dispatcher.py

+    def get_call_template(self, args, kws):
+        """
+        Get a typing.ConcreteTemplate for this dispatcher and the given
+        *args* and *kws* types.  This allows to resolve the return type.


Perhaps This enables the resolving of the return type.?

stuartarchibald · 2018-06-12T12:36:39Z

numba/ir_utils.py

+
+def fill_callee_prologue(block, inputs, label_next):
+    """
+    Fill a new block *block* that unwraps argument using names in *inputs* and


s/argument/arguments/

stuartarchibald · 2018-06-12T12:37:00Z

numba/ir_utils.py

+def fill_callee_prologue(block, inputs, label_next):
+    """
+    Fill a new block *block* that unwraps argument using names in *inputs* and
+    then jump to *label_next*.


s/jump/jumps/

stuartarchibald · 2018-06-12T12:42:30Z

numba/transforms.py

-        return block
+    entry_block = blocks[loopinfo.callfrom]
+    scope = entry_block.scope
+    loc = entry_block.loc

    def make_epilogue():


Is this function now dead? Think ir_utils.fill_callee_epilogue does the functional equivalent?

stuartarchibald · 2018-06-12T12:50:50Z

numba/transforms.py

+
+
+def _get_with_contextmanager(func_ir, blocks, blk_start):
+    """Get the global object used for as the context manager


stuartarchibald · 2018-06-12T12:53:42Z

numba/transforms.py

+    counters.pop(ir.Del, None)
+    # There MUST NOT be any other statements
+    if counters:
+        raise errors.CompilerError(


Can a ref to loc be easily supplied here such that the location will be pointed at by the error handlers?

Added loc

stuartarchibald · 2018-06-12T12:54:44Z

numba/transforms.py

+            )
+
+
+def _cfg_nodes_in_region(cfg, region_begin, region_end):


Wonder whether this should perhaps go in ir_utils or controlflow ?

There are a number of things in transforms.py that can go into other modules. I want to defer that task to later.

stuartarchibald · 2018-06-12T12:56:01Z

numba/transforms.py

+    for s, e in withs:
+        if s not in doms[e]:
+            msg = "Entry of with-context not dominating the exit."
+            raise errors.CompilerError(msg)


Can a loc be supplied to these two errors?

stuartarchibald · 2018-06-12T13:01:46Z

numba/withcontexts.py

+
+def _bypass_with_context(blocks, blk_start, blk_end, forwardvars):
+    """Given the starting and ending block of the with-context,
+    replaces a new head block that jumps to the end.


Should this be replaces the head block with a new block that jumps to the end.?

stuartarchibald · 2018-06-12T13:02:18Z

numba/withcontexts.py

+
+
+class _CallContextType(WithContext):
+    """A simple context-manager that tells the compiler lift the body of the


s/compiler/compiler to/

stuartarchibald · 2018-06-12T13:39:59Z

Just done a bit of manual testing... some items for discussion

Seems like ir.EnterWith is not understood in object mode, or @vectorize, or @stencil and I imagine also @guvectorize.
Assuming it is desirable to fix 1. (which it may not be, in which case a more graceful error should be produced), the somewhat bizarre situation arises where object mode execution is potentially lifting out with-blocks as functions that are also running in object mode (cf. Loop lifting has same issue). I think this sort of thing might trigger it:

def objmode_with_objmode():
  object()
  with call_context:
    object()
  return 1

Parfors is very broken. e.g.

def lift():
  with call_context:
    pass
print(njit(parallel=True)(lift)())

gives:

  File "<path>/numba/numba/stencilparfor.py", line 49, in run
    if isinstance(call_list[0], StencilFunc):
IndexError: Failed at nopython (nopython frontend)
Failed at nopython (convert to parfors)
list index out of range

which was somewhat unexpected as there's no stencil anywhere near the source function!

Finally, is this to be documented in the developer documentation, or should that wait until more
features are added and the API etc stabilizes? Also, does this impact annotations, I don't think it will but am mindful that not breaking #2793 would be good!

@stuartarchibald

as per comments by @stuartarchibald

sklam · 2018-06-12T15:44:35Z

#3017 (comment) raised important concerns. Given how this patch is not adding any user facing feature yet, I am ok to delay the merge until some of the concerns are resolved even if it will not make it this release cycle. Particularly, the breaking of parfor features should be addressed first.

As for the objectmode support, there were no support for contextmanager in objectmode before. It will fail in the bytecode analysis. I think it is acceptable to skip objectmode support for this PR and just do a better errmsg for it. Future patches should implement the objectmode support.

ehsantn · 2018-06-18T14:19:47Z

@sklam the Parfor-related issue was a simple bug that I just fixed.

# Conflicts: # numba/ir.py # numba/ir_utils.py

sklam · 2018-07-26T16:36:10Z

Oh, looks like I need to resolve the loc review comments first.

sklam · 2018-07-26T20:24:09Z

This is ready for review now

stuartarchibald · 2018-08-08T12:40:50Z

numba/ir.py

+        contextmanager : IR value
+        begin, end : int
+            The beginning and the ending offset of the with-body.
+        loc : int


loc is a ir.Loc instance?

stuartarchibald · 2018-08-08T12:58:35Z

numba/tests/test_withlifting.py

+            njit(liftcall4)()
+        # Known error.  We only support one context manager per function
+        # for body that are lifted.
+        self.assertIn("re-entrant", str(raises.exception))


Any view on doing something with non-existent context managers? I just accidentally forgot to import the call_context, but this equally applies to e.g.

from numba import njit def bar(): with unknown_ctx: pass njit(bar)()

which gives the same fairly spectacular traceback (as the one if you forget the import) ending with:

<path>/numba/numba/transforms.py in with_lifting(func_ir, typingctx, targetctx, flags, locals) 293 _legalize_with_head(blocks[blk_start]) 294 cmkind = _get_with_contextmanager(func_ir, blocks, blk_start) --> 295 sub = cmkind.mutate_with_body(func_ir, blocks, blk_start, blk_end, 296 body_blocks, dispatcher_factory) 297 sub_irs.append(sub) AttributeError: Failed at nopython (nopython frontend) 'object' object has no attribute 'mutate_with_body'

i think this should be fixed in this PR

stuartarchibald · 2018-08-08T13:04:15Z

Thanks for the fixes @sklam I made a couple more comments but am of the view that this can be merged whilst acknowledging that there is more work to be done/this is experimental and incremental development is expected.

stuartarchibald

Thanks for the further fixes @sklam, one minor comment to resolve then this can be merged as convenient. Thanks again.

stuartarchibald · 2018-08-13T23:28:06Z

numba/transforms.py

+                    )
+            if not hasattr(ctxobj, 'mutate_with_body'):
+                raise errors.CompilerError(
+                    "Unsupported use of contextmanager",


Should that be Unsupported context manager in use, I think at this point the context manager statement has been identified and this is just duck type testing if it has an attr specific to Numba and is therefore supported?

Agree and fixed

stuartarchibald · 2018-08-14T15:44:57Z

Thanks for 995fdda @sklam, this can be merged as convenient.

stuartarchibald · 2018-08-14T19:42:50Z

@sklam tests are failing, think the error message update needs reflecting in the unit test catching the error. Thanks.

sklam · 2018-08-14T20:01:31Z

oops

stuartarchibald · 2018-08-14T22:54:35Z

Thanks for fixing. Merging.

sklam added 19 commits May 1, 2018 17:30

Begin work on with-lifting

4726b16

Working on with-lifting

a5e8024

Extract and bypass with-contexts

b156339

Add EnterWith IR statement

4adebcd

Add context-manager that bypasses block of code

2fd0705

Remove ir.Metadata

f9a2205

Fix for py2

bb88fa8

Make bypass context forward variables properly

94dfb17

and some cleanup

Work on with-context that lifts the body as a new function

05c6e32

Finish transformation for calling with-lifted block

2f0a6cc

More tests

cf39c3d

Fix py2 cyclic import

326ce89

Clean up

3cbc3da

Deduplicate code

92c4c71

Fix comment

0ffd5f1

Merge branch 'master' into enh/withlifting

7a3ef96

# Conflicts: # numba/ir_utils.py

Complete TODOs

ebd6c23

Fix unicode problem

d55f41e

Fix py2 cyclic import

b985985

seibert requested a review from stuartarchibald June 7, 2018 13:52

stuartarchibald reviewed Jun 12, 2018

View reviewed changes

Fix docstrings and remove dead code

0ea643e

as per comments by @stuartarchibald

fix stencil call matching

6156380

stuartarchibald added this to the Numba 0.40 RC milestone Jun 20, 2018

Merge branch 'master' into enh/withlifting

fe6f776

# Conflicts: # numba/ir.py # numba/ir_utils.py

sklam added the 3 - Ready for Review label Jul 25, 2018

sklam mentioned this pull request Jul 25, 2018

Objmode with-block #3166

Merged

2 tasks

sklam added 2 commits July 26, 2018 15:13

Add source locations to error raised

8420e5b

Add more loc

8b7e40d

stuartarchibald reviewed Aug 8, 2018

View reviewed changes

Add test on invalid contextmanager

f2b760b

stuartarchibald reviewed Aug 13, 2018

View reviewed changes

Improve error msg

995fdda

stuartarchibald approved these changes Aug 14, 2018

View reviewed changes

Fix test

a0b1f72

stuartarchibald merged commit 6555067 into numba:master Aug 14, 2018

sklam deleted the enh/withlifting branch August 23, 2018 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add facility to support with-contexts #3017

Add facility to support with-contexts #3017

sklam commented Jun 6, 2018

codecov-io commented Jun 6, 2018 •

edited

stuartarchibald left a comment

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

sklam Jul 26, 2018

stuartarchibald Jun 12, 2018

sklam Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald Jun 12, 2018

stuartarchibald commented Jun 12, 2018

sklam commented Jun 12, 2018

ehsantn commented Jun 18, 2018

sklam commented Jul 26, 2018

sklam commented Jul 26, 2018

stuartarchibald Aug 8, 2018

stuartarchibald Aug 8, 2018

sklam Aug 9, 2018

stuartarchibald commented Aug 8, 2018

stuartarchibald left a comment

stuartarchibald Aug 13, 2018

sklam Aug 14, 2018

stuartarchibald commented Aug 14, 2018

stuartarchibald commented Aug 14, 2018

sklam commented Aug 14, 2018

stuartarchibald commented Aug 14, 2018



		def _get_with_contextmanager(func_ir, blocks, blk_start):
		"""Get the global object used for as the context manager



		class _CallContextType(WithContext):
		"""A simple context-manager that tells the compiler lift the body of the

Add facility to support with-contexts #3017

Add facility to support with-contexts #3017

Conversation

sklam commented Jun 6, 2018

codecov-io commented Jun 6, 2018 • edited

Codecov Report

stuartarchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartarchibald commented Jun 12, 2018

sklam commented Jun 12, 2018

ehsantn commented Jun 18, 2018

sklam commented Jul 26, 2018

sklam commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartarchibald commented Aug 8, 2018

stuartarchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartarchibald commented Aug 14, 2018

stuartarchibald commented Aug 14, 2018

sklam commented Aug 14, 2018

stuartarchibald commented Aug 14, 2018

codecov-io commented Jun 6, 2018 •

edited