Fixing issue 7693

[luk-f-a]

Fixes the lowering of setitem of a nestedarray into the nestedarray field of a record. Changes to the generated LLVM IR in those cases, to prevent the metadata (strides, etc) of the source being copied into the data buffer of the destination array.

fixes #7693

[gmarkall]

This also fixes the other reproducer from 7693

import numpy as np
from numba import njit

dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True)


@njit
def ex1(dest):
    tmp = (np.arange(3) + 1).astype(np.int16)
    dest['array1'] = tmp 


dest = np.empty(5, dtype=dest_dtype)
expected = np.empty(5, dtype=dest_dtype)
ex1(dest[0])
ex1.py_func(expected[0])

print(expected[0])
print(dest[0])

which gives:

$ python ex1.py 
([1, 2, 3],)
([1, 2, 3],)

with this PR.

[gmarkall]

Thanks for looking at this so far! I hope you don't mind that I pushed a fix for the flake8 issues, just to make it a little clearer where the "true" failures are.

[gmarkall]

Note that the failures are all variations on the same issue:

TypeError: Can only insert float at [0] in [6 x float]: got i8*

where [6x float] is the data for the nested array.

[gmarkall]

@luk-f-a I have added the two reproducers we have for #7693 as tests as well - they seem to be passing for me locally.

[luk-f-a]

@luk-f-a I have added the two reproducers we have for #7693 as tests as well - they seem to be passing for me locally.

oh, I didn't see the message and added one too. I'll remove it later. For now, I'm trying to fix the tests that the previous push broke. Some of them are passing locally, I hope I've fixed all of them.

[luk-f-a]

open question for a reviewer:

• is the if in function record_setattr the best way to product the special behaviour needed for nested arrays? That's as close to LLVM IR as I could take it. Alternatively, it could be done upstream from there, maybe here?

  numba/numba/np/arrayobj.py

  Lines 2630 to 2640 in bf480b9

  	@lower_builtin('static_setitem', types.Record, types.StringLiteral, types.Any)
  	def record_static_setitem_str(context, builder, sig, args):
  	"""
  	Record.__setitem__ redirects to setattr()
  	"""
  	recty, _, valty = sig.args
  	rec, idx, val = args
  	getattr_sig = signature(sig.return_type, recty, valty)
  	impl = context.get_setattr(idx, getattr_sig)
  	assert impl is not None
  	return impl(builder, (rec, val))

[gmarkall]

gpuci run tests

[gmarkall]

@luk-f-a Many thanks for the fixes - from a preliminary glance this is looking promising - do you consider this non-draft / ready for review now?

• is the if in function record_setattr the best way to product the special behaviour needed for nested arrays?

I think it's as good as we can do given the way the NestedArray abstraction is - I don't see a problem with this pattern. It's also consistent with some of the other implementation from #7359.

• Alternatively, it could be done upstream from there, maybe here?

I think it's better to keep it where it is because some code sets attrs on structured types rather than using setitem, and the current implementation will handle both cases.

[luk-f-a]

thanks for the feedback, I'll remove the duplicated test and promote to non-draft

[luk-f-a]

@gmarkall do the new tests need a cuda version?

[gmarkall]

@gmarkall do the new tests need a cuda version?

Ah, that's a good point - I'm not a sure if this woudl work with regualr arrays in CUDA, so I#ll have a quick check to see.

[gmarkall]

gpuci run tests

[gmarkall]

It turns out that the new tests do work in CUDA, so I've added them.

[gmarkall]

gpuci run tests

[gmarkall]

@stuartarchibald Many thanks for the review and suggested fixes. I also thought this might fail assigning an array to a whole array's worth of nested arrays, but it fails in typing as required, so I've just added an additional test to ensure that we continue to produce a typing error in this case (instead of it sneaking past typing and miscompiling or raising a lowering error.

I've also merged from master, but there were no conflicts.

[gmarkall]

gpuci run tests

[gmarkall]

The gpuCI fail should be resolved when #7740 is merged.

[gmarkall]

gpuci run tests

[stuartarchibald]

Thanks for the patch and fixes, looks good.

[stuartarchibald]

@gmarkall please could you review/approve too RE patch 364fe7b from #7712 (comment) as I'm author. Thanks.

[gmarkall]

Approved re patch 364fe7b, of which @stuartarchibald is the author.

[stuartarchibald]

Approved re patch 364fe7b, of which @stuartarchibald is the author.

Thanks @gmarkall

[esc]

Build numba_smoketest_cuda_yaml_114 has started

[esc]

Build numba_smoketest_cuda_yaml_114 has started

all green

[stuartarchibald]

Build numba_smoketest_cuda_yaml_114 has started

all green

Thanks @esc

[stuartarchibald]

Thanks for all your work on this @luk-f-a and @gmarkall, glad to see this going into 0.55.1!