Fix for head_index_parallel test failing if executed twice and with MPI #127
Conversation
[BUG] Fixed problems with generation parquet files (IntelPython#93)
Fixing issue with named series handling in fillna (IntelPython#95)
Merge from public
Merging commits from public repo
Merge from public repo
Merge from public repo
Merge from public repo
Merge from public repo
Add tests for Series arithmetic and comparison methods and fix Series…
Merge from public repo
Merge from public repo
Merge from public repo
The test 'test_series_head_index_parallel2' from TestSeries was hanging when executed twice on Windows with MPI run and number of processors equal to 3: mpiexec -n 3 python -u -m unittest $TESTS_TO_RUN where TESTS_TO_RUN="hpat.tests.TestSeries.test_series_head_parallel1 unittest hpat.tests.TestSeries.test_series_head_parallel1" The problem was in wrong calculation of n_chars in const_slice_getitem overload for StringArrayType, which was not correctly reduced by all processors and had wrong value in some cases. The fix is to simplify the implementation and to have workers decide whether their part of the array has to be sent or not basing on the position of their subset in the total array and the size of requested slice.
kozlov-alexey
requested review from
shssf,
AlexanderKalistratov,
fschlimb,
Hardcode84,
densmirn and
Vyacheslav-Smirnov
|
@shssf OK, next time I won't. I just started fixing newly introduced issues and forgot to stop. :) |
| displs = np.empty(1, np.int32) | ||
| displs_char = np.empty(1, np.int32) | ||
| displs = np.empty(n_loc, np.int32) | ||
| displs_char = np.empty(n_loc, np.int32) | ||
|
|
There was a problem hiding this comment.
displs and displs_char are needed on root only and will be set properly in the following if-condition.
This is just a dummy for all other ranks to keep the code simple.
A proper solution would make c_gatherv accept None or 0 for non-root ranks. This might require some extra work like casting...
BTW: n_loc is definitely not the right value anyway.
There was a problem hiding this comment.
@fschlimb Yes, I left it accidentally, it's not needed at all. I can change it to 0 size array, since the size doesn't really matter, or we can even remove these two lines as redundant. I'm not sure I got how to update c_gatherv. It accepts void*, but in gatherv_overload we have to keep the type consistent, so if we assign something to displs it it should have the same type as in the root branch. Shall I remove these lines?
There was a problem hiding this comment.
I don't think you can remove them because you need something type-stable to pass it to c_gatherv. If a zero-sized array works that'd be great.
If numba does not automatically cast 0 to a pointer when the C-function expects a void* you can write an intrinsic which returns a ((void*)0) and use that in the function you @jit/@overload. Maybe like this
@intrinsic
def mk_int32_null(typingctx):
def codegen(context, builder, sig, args):
zero = context.get_constant(types.int32, 0)
return builder.inttoptr(zero, llvmlite.ir.IntType(32))
return numba.types.int32, codegen
It's probably not worth the effort, though. You will probably need yet another layer between the caller and the ExternFunction. In particular if passing an empty array works we should just use that.
There was a problem hiding this comment.
It looks like else branch can be remove (I tried and test still passes). Inserting "print(my_arr)" into it shows empty array on non root ranks (i.e. it's not even needed to assign something in the else branch), looks like Numba default constructs the object, once it's able to infer the type? I failed to find where the magic comes from in the IR. I suggest keeping latest commit with zero size array which works too.
| @@ -872,14 +859,38 @@ def test_impl(S): | |||
| self.assertEqual(count_array_REPs(), 0) | |||
| self.assertEqual(count_parfor_REPs(), 0) | |||
|
|
|||
| def test_series_dist_input2(self): | |||
| def test_impl(S): | |||
There was a problem hiding this comment.
Pls add a doc-string saying what it's testing.
| self.assertEqual(count_array_REPs(), 0) | ||
| self.assertEqual(count_parfor_REPs(), 0) | ||
|
|
||
| def test_series_dist_input3(self): |
There was a problem hiding this comment.
Pls add a doc-string saying what it's testing.
| # get total characters for allocation | ||
| n_chars = np.uint64(0) | ||
| if k > count: | ||
| if k > start: |
There was a problem hiding this comment.
Looks like the code from if-statement is also valid for else-statement. I mean we can avoid if-else branching here and leave only code from the if-statement.
There was a problem hiding this comment.
@densmirn That's true, thank you. But I think keeping if statement simplifies code. I will remove the redundant "max(k - start, 0)" in the first branch though, i.e. will make it this:
if k > start:
# if slice end is beyond the start of this subset we have to send our elements
my_end = min(count, k - start)
my_arr = arr[:my_end]
else:
my_arr = arr[:0]
1ace757
to
8dc13c7
Compare
|
manually triggered azure succeeded: https://dev.azure.com/IntelPython/HPAT/_build/results?buildId=45 |
…PI (IntelPython#127) * Fix for Series.head and distribution of StringArrayType as a const_slice (fixing wrong calculation of n_chars in const_slice_getitem)
No description provided.