Fix: ht.array constructor respects implicit torch device when copy is set to false

[JuanPedroGHM]

Due Diligence

• General:
  • title of the PR is suitable to appear in the Release Notes
• Implementation:
  • unit tests: all split configurations tested
  • unit tests: multiple dtypes tested
  • documentation updated where needed

Description

ht.array constructor deleted original object torch device when the copy flag is set to False.

Issue/s resolved: #1321

Changes proposed:

• Earlier check in ht.array factory for the original object's device.
• Added a test for ht.array using torch tensor, on different devices (when changing the env variable), and with copy set to false.

Type of change

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

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

yes?

[ghost]

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

Legend

[github-actions]

Thank you for the PR!

[codecov]

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.05%. Comparing base (7acf5be) to head (7fcf288).

Files	Patch %	Lines
heat/core/factories.py	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1363      +/-   ##
==========================================
- Coverage   92.06%   92.05%   -0.01%     
==========================================
  Files          80       80              
  Lines       11942    11950       +8     
==========================================
+ Hits        10994    11001       +7     
- Misses        948      949       +1     

Flag	Coverage Δ
unit	92.05% <89.47%> (-0.01%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtar]

Thank you for tackling this issue @JuanPedroGHM. I created a table of the allowed parameter combinations for clarity:

copy	split	is_split	allowed? ; expected behaviour
False	Int	Int/None	🔴 ; exception
False	None	Int/None	🟢 ; reuse memory
True	Int	Int	🔴 ; exception
True	None	Int	🟡 : ; copy local memory (implemented?)
True	Int/None	None	🟢 ; copy memory
None	Int	Int	🔴 ; exception
None	Int	None	🟢 ; reuse memory as much as possible before the distributing and copying
None	None	Int	🟡 ; reuse memory if possible, local copy otherwise (implemented?)
None	None	None	🟢 ; reuse memory if possible, copy otherwise

What do you think?

[JuanPedroGHM]

So I did a bit of a brute force grid search and found all this instances that to me are incorrect behavior from heat. Let me know what you think:

Current inconsistent behavior (Summary):

• When creating from a {list, numpy, torch}, if is_split is set, and split is not, the split property of the new array is set to that split, but the array is not actually split.
  • This also happens for heat, when the original array is not split.
  • Expected result: the is_split argument should be ignored, as they are not split originally. New DNDArray should have the same split indicated as the split arg
• When both split and is_split are set, it always throws an exception. Could be changed to ignore in some cases, for list, numpy, torch arrays for example, as they are not split by default.
• When the original obj is a heat DNDArray that has already been split, the behavior is very inconsistent.
  • The split property of the new array is incorrect a lot of times, even if the split is done right.
  • When the original array is split, and the argument split of ht.array is set, the new array is split a second time. Probably not what we want.
• When the og object is a heat dndarray, the split/is_split exclusivity will not throw and error as long as all the properties from the old and new array match (exits on the first if clause).
• No proper test with multiple devices yet, Horeka and Haicore are unavailable.

Attached is also a full table with the data.

bug_results.md

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[JuanPedroGHM]

Changes:

• Raised exceptions when using the ht.array method would resplit the existing and already split DNDArray. If the original DNDArray is not split (obj.split is None), using ht.array will return a split object. This is to ensure compatibility with the rest of the heat code.
• The check if the arguments split and is_split was moved higher to ensure it cannot be circumvented with the right choice of arguments.
• Added additional tests that check those combinations of arguments.

New table:
bug_results.md

[github-actions]

Thank you for the PR!

[mtar]

Hi @JuanPedroGHM, I made some suggestions for better understanding.

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[mrfh92]

From my point of view this looks fine; in particular, I guess that the additional error messages will be very helpful (in particular since I have already made a mistake regarding split and is_split myself).
Thanks 👍

[ClaudiaComito]

Thanks a lot @JuanPedroGHM .

The CI matrix must be updated here before we can merge.

Also, just to be safe, can you test that the tutorials still behave as expected after these changes? Internals would be a good one to check (also feel free to add content there, I don't think we've addressed the copy attribute at all).

Thanks again!

[github-actions]

Thank you for the PR!

[JuanPedroGHM]

Thanks a lot @JuanPedroGHM .

The CI matrix must be updated here before we can merge.

Also, just to be safe, can you test that the tutorials still behave as expected after these changes? Internals would be a good one to check (also feel free to add content there, I don't think we've addressed the copy attribute at all).

Thanks again!

Thanks for the suggestion @ClaudiaComito ! I ran the tutorial (on my laptop, so single gpu and without the datasets) and they ran perfectly. I did find one error in the internals tutorial with an undefined variable "target_map", which I corrected with the last commit.

I also added a small example to the interoperability section in tutorial 2 about using the copy keyword.

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[ClaudiaComito]

Thanks again, next try...

[github-actions]

Thank you for the PR!

[github-actions]

Backport failed for release/1.3.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release/1.3.x
git worktree add -d .worktree/backport-1363-to-release/1.3.x origin/release/1.3.x
cd .worktree/backport-1363-to-release/1.3.x
git switch --create backport-1363-to-release/1.3.x
git cherry-pick -x b1203e745f5045537794c6581728c78444d29776 f1b568b323680ecf84b312a0355ea6061d7398bc 5525bb6f5d1b415672bc3a9ec96bac933c18ce5e b1ef6f95547493133f8f650a42b5ed6fe12097fb 3a97d5f9dae0457c1bc0a32670d3ed6349bf20e0 6f878faf1f8565ca5ad745a38c90891cf8d7caf7 c5052008fc70093c6cd98282ade4da3052c092d2 dffa4fad0141a90adaa8c9858d307d7e99c84a58 a84ba76fae2ee107ba71e898f73b7e1babbe4043