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

Implement literal dictionaries and lists. #5946

Merged
merged 26 commits into from Jul 15, 2020

Conversation

stuartarchibald
Copy link
Contributor

As title.
Permits compilation of things like:

@njit
def foo():
  d = {'a': 1,
       'b': 2j,
       'c': 'STRING',
       'd': np.zeros(5),
       'e': ['list', 1, 'of', 2, 'mixed', 3, 'types']}

WIP, opened for CI.

@sklam
Copy link
Member

sklam commented Jul 2, 2020

From out-of-band discussion:

  • This feature is pushing the limit of type inference
  • We speculate that could be problem with over-specialization due to the capturing of initial-value as part of the type.
  • Planned actions:
    • benchmark for problems after merging
    • fix the overspecialization in a new PR

Copy link
Member

@sklam sklam 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 done a first pass review. There are a few more details I want to look at.

numba/core/types/containers.py Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
numba/cpython/tupleobj.py Outdated Show resolved Hide resolved
numba/core/typeinfer.py Outdated Show resolved Hide resolved
@stuartarchibald
Copy link
Contributor Author

479b425 is failing with:

FAIL: test_dict_unify (numba.tests.test_dictobject.TestLiteralStrKeyDict)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/1/s/numba/tests/test_dictobject.py", line 1983, in test_dict_unify
    self.assertEqual(foo(100), 'd')
AssertionError: 'CAT' != 'd'
- CAT
+ d

think literal dictionary unification is probably wrong

@stuartarchibald stuartarchibald self-assigned this Jul 8, 2020
@stuartarchibald
Copy link
Contributor Author

#5946 (comment) is fixed by b17f841 on wards.

@stuartarchibald stuartarchibald marked this pull request as ready for review July 9, 2020 12:47
@stuartarchibald
Copy link
Contributor Author

@sklam this still has over-specialisation issues but is probably in a position where it's close enough to working to be reviewable! Thanks.

@sklam sklam added the 4 - Waiting on reviewer Waiting for reviewer to respond to author label Jul 13, 2020
@sklam
Copy link
Member

sklam commented Jul 13, 2020

The new changes are looking good. Feature-wise, it will address the use-cases for https://gist.github.com/sklam/4f08128860485485c7b3dbd2c9149e07.

I did notice that it is difficult to search for the added feature in the current doc. We should probably address this separately.

I will want to have another look at the specific details in the new overload_method.

numba/cpython/listobj.py Outdated Show resolved Hide resolved
numba/cpython/listobj.py Show resolved Hide resolved
numba/cpython/listobj.py Show resolved Hide resolved
numba/cpython/listobj.py Outdated Show resolved Hide resolved
@lower_cast(types.DictType, types.DictType)
def cast_DictType_DictType(context, builder, fromty, toty, val):
# should have been picked up by typing
return val
Copy link
Member

Choose a reason for hiding this comment

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

untested

Copy link
Member

Choose a reason for hiding this comment

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

how do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think it's left over from trying to make literal dictionary types gracefully degrade, removed in 120ffce

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it is needed: https://dev.azure.com/numba/numba/_build/results?buildId=6009&view=logs&j=12ecc1d6-2964-5850-b419-89f950957853&t=abce8f77-76e0-5291-fb7e-5ca9f390cd04&l=9914 IIRC typed.Dict uses the list dtype and not its initial value, but the initial value is part of the type, so casts such as the one failing won't work without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted 120ffce in ad7343f

Copy link
Member

Choose a reason for hiding this comment

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

Cannot cast DictType[int32,int32]<iv={4: 2}> to DictType[int32,int32]<iv={1: 3}>:

why is that allowed? should it be unified to one without initial_value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this stems from two problems:

  1. No attempt at unification in the cast impl
    @intrinsic
    def _cast(typingctx, val, typ):
    """Cast *val* to *typ*
    """
    def codegen(context, builder, signature, args):
    [val, typ] = args
    context.nrt.incref(builder, signature.return_type, val)
    return val
    # Using implicit casting in argument types
    casted = typ.instance_type
    _sentry_safe_cast(val, casted)
    sig = casted(casted, typ)
    return sig, codegen
  2. DictType will only unify with another DictType if the other is "imprecise"
    def unify(self, typingctx, other):
    """
    Unify this with the *other* dictionary.
    """
    # If other is dict
    if isinstance(other, DictType):
    if not other.is_precise():
    return self

numba/typed/dictobject.py Outdated Show resolved Hide resolved
numba/core/interpreter.py Show resolved Hide resolved
numba/core/interpreter.py Outdated Show resolved Hide resolved
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jul 14, 2020
@sklam
Copy link
Member

sklam commented Jul 14, 2020

smoking at numba_smoketest_cpu_49

@sklam
Copy link
Member

sklam commented Jul 15, 2020

numba_smoketest_cpu_49 is failing but due to entry-point test (probably a llvmlite version pinning problem again).
I don't see other failures from my random sample.

Update: confirmed that all failed tests are entry-point tests due to llvmlite version pinning; thus this PR has essentially passed our smoketest.

@stuartarchibald stuartarchibald 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 Jul 15, 2020
@sklam sklam removed the 4 - Waiting on reviewer Waiting for reviewer to respond to author label Jul 15, 2020
@sklam sklam added BuildFarm Passed For PRs that have been through the buildfarm and passed 4 - Waiting on reviewer Waiting for reviewer to respond to author 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 Jul 15, 2020
@sklam sklam merged commit 71a1ece into numba:master Jul 15, 2020
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 BuildFarm Passed For PRs that have been through the buildfarm and passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants