Fix edge-case contiguity mismatch for Allgatherv

[ClaudiaComito]

Description

Function communication.mpi_type_and_elements_of() calculates type and number of elements that will be sent/received in an Allgatherv call. How to calculate the number of elements depends on whether the input object (most likely a torch Tensor) is contiguous.

In some edge cases, i.e. in case of singleton split dimension, torch.Tensor.is_contiguous() might return True on a process while it is False on others. This result in a mismatch of the send/recv elements among processes in Allgatherv and resulting deadlock (see #1057 ).

Issue/s resolved: #1057

Changes proposed:

• Introduce global is_contiguous boolean as kwarg for communication.as_buffer() and communication.mpi_type_and_elements_of(). It is set to False on all processes if object dimensions have been permuted, independently on the size of the dimension
• More deadlocks/errors came up running the tests, solved here:
  • removed unnecessary MPI calls for non-distributed cases in dndarray.resplit_(), manipulations.resplit(), linalg.matmul(), and tests
  • in test_suites.basic_tests.TestCase.assert_array_equal, local tensors are compared to the relevant slices of the numpy reference array, instead of gathering the distributed DNDarray every time. TODO: Same should be implemented in Avoid unnecessary gathering of distributed operand in mixed distributed/non-distributed logical functions #1064
  • ht.allclose now works on operands with different dtype as well. Related to ht.allclose should factor in the dtype of the inputs when determining the limits #889

Type of change

• Bug fix (non-breaking change which fixes an issue)

Memory requirements

NA

Performance

Due Diligence

• All split configurations tested
• Multiple dtypes tested in relevant functions
• Documentation updated (if needed)
• Title of PR is suitable for corresponding CHANGELOG entry

Does this change modify the behaviour of other functions? If so, which?

no

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[codecov]

Codecov Report

Merging #1058 (d62a64d) into release/1.2.x (6158fa9) will increase coverage by 0.03%.
The diff coverage is 94.28%.

@@                Coverage Diff                @@
##           release/1.2.x    #1058      +/-   ##
=================================================
+ Coverage          91.76%   91.80%   +0.03%     
=================================================
  Files                 65       65              
  Lines              10024    10075      +51     
=================================================
+ Hits                9199     9249      +50     
- Misses               825      826       +1     

Flag	Coverage Δ
unit	91.80% <94.28%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
heat/core/linalg/basics.py	95.44% <85.71%> (-0.09%)	⬇️
heat/core/communication.py	96.23% <100.00%> (+0.02%)	⬆️
heat/core/dndarray.py	96.79% <100.00%> (+0.01%)	⬆️
heat/core/logical.py	100.00% <100.00%> (ø)
heat/core/manipulations.py	98.63% <100.00%> (-0.01%)	⬇️
heat/core/tests/test_suites/basic_test.py	98.07% <100.00%> (+0.01%)	⬆️
heat/core/constants.py	100.00% <0.00%> (ø)
heat/core/trigonometrics.py	100.00% <0.00%> (ø)
heat/core/random.py	99.67% <0.00%> (+<0.01%)	⬆️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mtar]

Thank you @ClaudiaComito

You added extra lines for arrays with different splits when the number of MPI processes is one. It doesn't make much sense to pass an argument when the split won't take place on one process. What do you think about disallowing splits or automatically setting he value to None when only a single process is involved at array creation time? It would save us some tests/checks.

[mtar]

What happens if the value is different on the processes. How likely is it?

[ClaudiaComito]

Thanks @mtar, that's a great question. The obvious case in which this might happen is the permutation, and this is dealt with in this PR. Outside of that, we're simply falling back to the previous implementation.

I could add a global check that sets is_contiguous to False if the local contiguities are dishomogeneous.

[ClaudiaComito]

Thank you @ClaudiaComito

You added extra lines for arrays with different splits when the number of MPI processes is one. It doesn't make much sense to pass an argument when the split won't take place on one process. What do you think about disallowing splits or automatically setting he value to None when only a single process is involved at array creation time? It would save us some tests/checks.

This is a general discussion worth having, maybe not re: this bug fix.
My main argument against setting all splits to None when running on 1 MPI process, is that it will be confusing for users while they are testing their code (potentially on 1 process or even interactively).

Anyway, let's discuss it in a separate Issue.

As far as I'm concerned, I'm done with this PR.

[ClaudiaComito]

Thanks @mtar, that's a great question. The obvious case in which this might happen is the permutation, and this is dealt with in this PR. Outside of that, we're simply falling back to the previous implementation.

I could add a global check that sets is_contiguous to False if the local contiguities are dishomogeneous.

I've decided not to add (yet another) global check for contiguous status for now, as I can't think of the appropriate edge-case to test it. We are already testing for column-first memory layout operations. If anybody can think of something, let me know.