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

Objmode with-block #3166

Merged
merged 36 commits into from Sep 10, 2018
Merged

Objmode with-block #3166

merged 36 commits into from Sep 10, 2018

Conversation

sklam
Copy link
Member

@sklam sklam commented Jul 25, 2018

(Based on #3017)

@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

Merging #3166 into master will increase coverage by 0.07%.
The diff coverage is 89%.

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
+ Coverage   81.09%   81.17%   +0.07%     
==========================================
  Files         386      388       +2     
  Lines       76285    76921     +636     
  Branches     8575     8631      +56     
==========================================
+ Hits        61867    62440     +573     
- Misses      13100    13155      +55     
- Partials     1318     1326       +8

@seibert seibert added this to the Numba 0.40 RC milestone Aug 10, 2018
@sklam
Copy link
Member Author

sklam commented Aug 22, 2018

Merged #3230 into this patch.

@sklam sklam mentioned this pull request Aug 30, 2018
@sklam
Copy link
Member Author

sklam commented Aug 30, 2018

Feature is completed. Ready for early review. I will just be adding more tests for crazy cases; like looplifting calling into objmode and vice versa.

@ehsantn
Copy link
Collaborator

ehsantn commented Sep 2, 2018

@sklam this works great! A couple of minor issues:

  • Could you add developer docs for this feature's design? Also, more comments throughout the code could help.
  • Using numba.withcontexts.objmode_context doesn't work since get_ctxmgr_obj expects a Global.
  • If a variable's type is not defined by mistake, this throws a KeyError instead of proper error message.
  • Can objmode_context be exported at the top level (i.e. numba.objmode_context)?

@ehsantn
Copy link
Collaborator

ehsantn commented Sep 2, 2018

@DrTodd13 @fschlimb could you take a look at this feature as well (since it's a critical feature)?

@ehsantn
Copy link
Collaborator

ehsantn commented Sep 2, 2018

@sklam with this feature in place, I'm wondering if it is now easier to just declare a python function as object mode inside all njit functions. The user just provides type inference and Numba handles boxing/unboxing automatically. For example:

@infer_object_mode(foo)
class FooInfer(AbstractTemplate):
    def generic(self, args, kws):
        return signature(types.float64, *args)

Changes:
- reject list as inputs to objmode due to difficulty in handling reflection
- checks and enforces type annotation on outgoing vars
- checks invalid control-flow usage inside with-context
add docstring
test example in docstring
@sklam
Copy link
Member Author

sklam commented Sep 7, 2018

Notes on new changes:

  • objmode_context renamed to objmode; exposed to top-level ns.
    no need to worry about the "objmode" target because the objmode context is marking a region for that target.
  • numba/objmode.py is not numba/pylowering.py (which contains class PyLower) to avoid name collision.
  • @stuartarchibald 's torture tests are included. Some are addressed/fixed and some are too hard to support; i.e. list and function types are not allowed to go into a objmode context. For the closure-using-objmode one, I have to mark it as expected-failure as there are no ways to handle it.
  • new docs. most of the docs are autogen from docstring in help(numba.objmode).

@sklam
Copy link
Member Author

sklam commented Sep 7, 2018

@ehsantn , regarding #3166 (comment), it's definitely easier now. I think one can also just use @overload; e.g.:

@overload(mytoy)
def mytoy_ov(x, y):
    def impl():
        with objmode(out=...):
               out = mytoy(x, y)
        return out
    return impl

@sklam sklam changed the title [WIP] Objmode with-block Objmode with-block Sep 7, 2018
@ehsantn
Copy link
Collaborator

ehsantn commented Sep 7, 2018

Interesting solution. Makes sense.

@seibert seibert mentioned this pull request Sep 10, 2018
17 tasks
- with-block cannot use incoming list object.
- with-block cannot use incoming function object.
- with-block cannot ``yield``, ``break``, ``return`` or ``raise`` such that
the execution will leave the with-block immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

need to indent this line so that sphinx doesn't think the list has ended

- with-block cannot ``yield``, ``break``, ``return`` or ``raise`` such that
the execution will leave the with-block immediately.
- the ``objmode()`` function must be directly referenced as a global.
i.e. ``with numba.objmode():`` doesn't work.
Copy link
Contributor

Choose a reason for hiding this comment

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

same need for indent here

@seibert
Copy link
Contributor

seibert commented Sep 10, 2018

Current failures:

  • Minor sphinx rendering warning in docstring that needs to be fixed
  • Crash bug on 32-bit Linux during testing. Not sure about cause...

@seibert seibert merged commit 35d9cdc into numba:master Sep 10, 2018
@sklam sklam deleted the enh/withobjmode branch September 11, 2018 02:05
@seibert seibert moved this from In Progress to Done in Major Features Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants