`ht.array`, closed loophole allowing `DNDarray` construction with incompatible shapes of local arrays #1034

Mystic-Slice · 2022-10-21T05:07:33Z

Description

The reuse of gshape array as the receiving buffer made the code a bit too difficult to understand. So, created a new array neighbour_shape to hold, well...., the neighbour's shape.

The use of MPI.SUM, to find if any process has encountered a mismatch in local shapes with its neighbour, leads to errors because of integer limitations. When two or more processes find a mismatch, the huge negative values are added together and I guess because of integer overflow, they disappear into the unknown and finally, we are left with a positive value. So, it does not create any errors when it should. This can be easily solved by using MPI.MIN instead.

One more thing that I don't understand is, why gshape[i] is allowed to be one less than lshape[i] in line 405. I know there must be a reason for it. I just don't understand why.

Code snippet:

import heat as ht
import torch

rank = ht.communication.MPI_WORLD.rank
dim = rank+1

larray = [[0]*dim]*dim
print("Rank", rank, larray)

d = ht.array(larray, is_split=0)

# literally any operation that involves communication 
# between processes will crash
# like this simple print statement
print(d)

Due Diligence

All split configurations tested
Multiple dtypes tested in relevant functions
Documentation updated (if needed)
Updated changelog.md under the title "Pending Additions"

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

no

skip ci

* wip: Initial release draft and changelog updater actions configuration * doc: pr title style guide in contibuting.md * ci: improved release draft templates * ci: extra release draft categories * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* doc: parallel tutorial note metioning local and global printing * doc: extenden local print note with ``ht.local_printing()`` * Fix typo * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Updated the tutorial document. 1. Corrected the spelling mistake -> (sigular to single) 2. Corrected the statement -> the number of dimensions is the rank of the array. 3. Made 2 more small changes. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix typo Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

updates: - [github.com/psf/black: 22.3.0 → 22.6.0](psf/black@22.3.0...22.6.0)

…t + Added test

* Check for split in `__reduce_op` * Check whether x is distributed Co-authored-by: mtar <m.tarnawa@fz-juelich.de> Co-authored-by: mtar <m.tarnawa@fz-juelich.de> Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com>

* Update ci worflow action * Update codecov.yml

* Fix `all` * Fix `any` * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add distributed tests * Expanded tests for combination of axis/split axis Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com> Co-authored-by: mtar <m.tarnawa@fz-juelich.de>

updates: - [github.com/psf/black: 22.8.0 → 22.10.0](psf/black@22.8.0...22.10.0)

…e-config [pre-commit.ci] pre-commit autoupdate

ghost · 2022-10-21T05:09:23Z

👇 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 · 2022-10-21T05:17:35Z

Codecov Report

Merging #1034 (aeb9ef3) into main (ae9dd50) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1034   +/-   ##
=======================================
  Coverage   91.68%   91.68%           
=======================================
  Files          65       65           
  Lines        9978     9978           
=======================================
  Hits         9148     9148           
  Misses        830      830

Flag	Coverage Δ
unit	`91.68% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
heat/core/factories.py	`100.00% <100.00%> (ø)`

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

ClaudiaComito

Thanks a lot @Mystic-Slice for tackling this. For me it's good to go, just 2 small changes please:

Update PR title so that it makes sense in the automated changelog.
Update error message (see in-line comment)

Again, great job and thank you so much!

ClaudiaComito · 2022-10-24T10:49:04Z

heat/core/factories.py

-        reduction_buffer = np.array(gshape[is_split])
-        comm.Allreduce(MPI.IN_PLACE, reduction_buffer, MPI.SUM)
+        reduction_buffer = np.array(neighbour_shape[is_split])
+        comm.Allreduce(MPI.IN_PLACE, reduction_buffer, MPI.MIN)
        if reduction_buffer < 0:
            raise ValueError("unable to construct tensor, shape of local data chunk does not match")


Let's rephrase this. How about "Unable to construct DNDarray. Local data slices have inconsistent shapes or dimensions." ? Or something similar

Historical note: in the very early phase, our DNDarray class was called Tensor. Any reference to "tensor" that doesn't refer to torch.tensor is outdated, feel free to update to DNDarray it if you spot one.

Thanks @ClaudiaComito. I have made the change.

ClaudiaComito · 2022-10-24T10:49:58Z

heat/core/factories.py

                for i in range(length):
                    if i == is_split:
                        continue
-                    elif lshape[i] != gshape[i] and lshape[i] - 1 != gshape[i]:


Good catch @Mystic-Slice !

Mystic-Slice · 2022-10-24T11:24:35Z

I have made the changes. @ClaudiaComito
Thanks for the review.

ClaudiaComito and others added 30 commits April 27, 2022 14:53

Merge branch 'release/1.2.x', set main version extension to "dev"

885f686

Update link to llnl MPI tutorial and merge branch 'release/1.2.x'

e30d739

Replace bug report MD template with form in view of further automation

08d0dcb

Fix bug report file name

545a694

Update bug_report.yml

295351c

Update bug_report.yml

91f8190

Update bug_report.yml

ef94a9b

Update bug_report.yml

4a1fc1a

Set write permissions for workflow

ee0ff4d

Update schedule

a1f0b1b

Update schedule

96506fa

Update schedule

385374a

Move pytorch version file out of workflows dir

bff6b2c

Update paths

4f59d69

[pre-commit.ci] pre-commit autoupdate

55fff46

updates: - [github.com/psf/black: 22.3.0 → 22.6.0](psf/black@22.3.0...22.6.0)

Push pytorch release update to release/1.2.x branch, not main

4fe322b

Update schedule

a52156b

Bypass on push trigger

229e172

Update schedule

6510ea4

Fix condition syntax

2a947a1

Fix syntax

6ccec82

On push trigger workaround

3cc0080

Update schedule

6c487d6

Update schedule

05a2acd

Merge branch 'main' into pre-commit-ci-update-config

2223ce8

Enable non-negative sample size

d39cbfe

Read min value directly from torch return object

105b905

mtar and others added 11 commits September 30, 2022 08:26

Update codecov.yml

0949a48

Fixed code checking for non-matching local shapes while using is_spli…

9b53a25

…t + Added test

Add section Google Summer of Code 2022

398ac0d

Add section "Array API"

db44c93

Mirror Repository and run GitHub CI at HZDR (#1032)

d5c14b7

* Update ci worflow action * Update codecov.yml

Merge branch 'main' into CleanUpIsSplit

756f43f

[pre-commit.ci] pre-commit autoupdate

5ba9b32

updates: - [github.com/psf/black: 22.8.0 → 22.10.0](psf/black@22.8.0...22.10.0)

Merge pull request #1033 from helmholtz-analytics/pre-commit-ci-updat…

ae9dd50

…e-config [pre-commit.ci] pre-commit autoupdate

Merge branch 'main' into CleanUpIsSplit

fe894b0

Mystic-Slice changed the title ~~Clean up is split~~ Fixed code that verifies lshape while using is_split Oct 21, 2022

Mystic-Slice mentioned this pull request Oct 21, 2022

Fixed code that verifies lshape while using is_split #1029

Closed

4 tasks

Mystic-Slice added chore GSoC labels Oct 21, 2022

ClaudiaComito added bug Something isn't working and removed chore labels Oct 24, 2022

ClaudiaComito requested changes Oct 24, 2022

View reviewed changes

Updated error message

aeb9ef3

Mystic-Slice changed the title ~~Fixed code that verifies lshape while using is_split~~ ht.array, closed loophole allowing DNDarray construction with incompatible shapes of local arrays Oct 24, 2022

ClaudiaComito added the PR talk label Oct 28, 2022

ClaudiaComito changed the base branch from main to release/1.2.x October 28, 2022 07:05

ClaudiaComito added the v1.2.1 label Oct 28, 2022

ClaudiaComito approved these changes Nov 3, 2022

View reviewed changes

ClaudiaComito merged commit 771a557 into release/1.2.x Nov 3, 2022

ClaudiaComito deleted the CleanUpIsSplit branch November 3, 2022 10:45

mtar removed the PR talk label Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ht.array`, closed loophole allowing `DNDarray` construction with incompatible shapes of local arrays #1034

`ht.array`, closed loophole allowing `DNDarray` construction with incompatible shapes of local arrays #1034

Mystic-Slice commented Oct 21, 2022 •

edited by ClaudiaComito

Loading

ghost commented Oct 21, 2022 •

edited by ghost

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading

ClaudiaComito left a comment

ClaudiaComito Oct 24, 2022

Mystic-Slice Oct 24, 2022

ClaudiaComito Oct 24, 2022

Mystic-Slice commented Oct 24, 2022

ht.array, closed loophole allowing DNDarray construction with incompatible shapes of local arrays #1034

ht.array, closed loophole allowing DNDarray construction with incompatible shapes of local arrays #1034

Conversation

Mystic-Slice commented Oct 21, 2022 • edited by ClaudiaComito Loading

Description

Code snippet:

Due Diligence

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

ghost commented Oct 21, 2022 • edited by ghost Loading

Legend

codecov bot commented Oct 21, 2022 • edited Loading

Codecov Report

ClaudiaComito left a comment

Choose a reason for hiding this comment

ClaudiaComito Oct 24, 2022

Choose a reason for hiding this comment

Mystic-Slice Oct 24, 2022

Choose a reason for hiding this comment

ClaudiaComito Oct 24, 2022

Choose a reason for hiding this comment

Mystic-Slice commented Oct 24, 2022

`ht.array`, closed loophole allowing `DNDarray` construction with incompatible shapes of local arrays #1034

`ht.array`, closed loophole allowing `DNDarray` construction with incompatible shapes of local arrays #1034

Mystic-Slice commented Oct 21, 2022 •

edited by ClaudiaComito

Loading

ghost commented Oct 21, 2022 •

edited by ghost

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading