Enhancement/select device #488

mtar · 2020-02-17T08:25:17Z

Description

This PR changes the names used for device selection for testing purposes. The old 'header' is now in BasicTest

Changes proposed:

rename Environment variable: DEVICE -> HEAT_TEST_USE_DEVICE
New value 'lcpu': global device set to gpu, local device to cpu
Rename variable: device -> torch_device
setupclass function in BasicTest
all unit test cases inherit from BasicTests

Type of change

Remove irrelevant options:

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Due Diligence

All split configurations tested
Multiple dtypes tested in relavent 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?

yes, all test functions

codecov · 2020-02-17T08:32:10Z

Codecov Report

Merging #488 into master will increase coverage by 0.93%.
The diff coverage is 98.18%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   96.48%   97.41%   +0.93%     
==========================================
  Files          75       75              
  Lines       15276    15019     -257     
==========================================
- Hits        14739    14631     -108     
+ Misses        537      388     -149

Impacted Files	Coverage Δ
heat/core/tests/test_suites/basic_test.py	`91.34% <57.14%> (-8.66%)`	⬇️
heat/core/linalg/tests/test_qr.py	`71.42% <66.66%> (+2.42%)`	⬆️
heat/core/tests/test_devices.py	`58.18% <79.41%> (-6.64%)`	⬇️
heat/core/tests/test_io.py	`93.05% <92.45%> (+1.50%)`	⬆️
heat/cluster/tests/test_kmeans.py	`100.00% <100.00%> (+10.41%)`	⬆️
heat/cluster/tests/test_spectral.py	`100.00% <100.00%> (+10.63%)`	⬆️
heat/core/linalg/tests/test_basics.py	`100.00% <100.00%> (+0.61%)`	⬆️
heat/core/linalg/tests/test_solver.py	`100.00% <100.00%> (+15.15%)`	⬆️
heat/core/operations.py	`92.73% <100.00%> (ø)`
heat/core/tests/test_arithmetics.py	`99.60% <100.00%> (+1.72%)`	⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc77d90...01eccf1. Read the comment docs.

heat/core/tests/test_devices.py

coquelin77

can you please also revert back to ht_device instead of heat_device? looking through all of these files is painful when the refactor touches so many lines. Also, the change feels arbitrary.

coquelin77 · 2020-03-21T08:55:32Z

CHANGELOG.md

@@ -15,6 +15,7 @@
 - [#470](https://github.com/helmholtz-analytics/heat/pull/470) Enhancement: Accelerate distance calculations in kmeans clustering by introduction of new module spatial.distance
 - [#478](https://github.com/helmholtz-analytics/heat/pull/478) `ht.array` now typecasts the local torch tensors if the torch tensors given are not the torch version of the specified dtype + unit test updates
 - [#479](https://github.com/helmholtz-analytics/heat/pull/479) Completion of spatial.distance module to support 2D input arrays of different splittings (None or 0) and different datatypes, also if second input argument is None
+- [#488](https://github.com/helmholtz-analytics/heat/pull/488) Enhancement: test device selection


can you expand on this? if someone was to read this they likely wouldnt understand what this means. Its okay to use two sentences here

coquelin77 · 2020-03-21T08:57:29Z

heat/core/tests/deviceselection.py

+    ht.use_device("cpu")
+    torch.cuda.set_device(torch.device(ht.gpu.torch_device))
+    torch_device = ht.gpu.torch_device
+    heat_device = ht.gpu


Can we make this a function instead? and then put it in the device.py file within core?

I don't think that it should be a library function. It is only needed in the tests.

i understand, but it goes against the programming model that we have everywhere else. and although this is only used in the unit tests at the moment, it may be useful as a tool for other developers if it is a library function.

mtar · 2020-03-23T13:28:50Z

can you please also revert back to ht_device instead of heat_device? looking through all of these files is painful when the refactor touches so many lines. Also, the change feels arbitrary.

ht_device is also a arbitrary name 🤷‍♂️
I changed it because I use torch_device now, but if you are fond of it I will revert it back.

When I think about it, a heat device is a heater 😀

coquelin77 · 2020-03-23T13:31:08Z

can you please also revert back to ht_device instead of heat_device? looking through all of these files is painful when the refactor touches so many lines. Also, the change feels arbitrary.

ht_device is also a arbitrary name man_shrugging
I changed it because I use torch_device now, but if you are fond of it I will revert it back.

When I think about it, a heat device is a heater grinning

honestly, i like ht_device because we typically have import heat as ht but yes it is 100% arbitrary

Markus-Goetz · 2020-04-29T10:50:56Z

heat/cluster/tests/test_kmeans.py

-    device = ht.gpu.torch_device
-    ht_device = ht.gpu
-    ht.torch.cuda.set_device(device)
+from heat.core.tests.test_suites.basic_test import BasicTest


Should probably be called TestCase as well

Should it be imported as TestCase or should the name be changed in general?

The name should be changed in general in my opinion

Markus-Goetz · 2020-04-29T10:51:21Z

heat/cluster/tests/test_kmeans.py

-class TestKMeans(unittest.TestCase):
+class TestKMeans(BasicTest):
+    @classmethod
+    def setUpClass(cls):


Does this need to be explicitly called here? Should be automatically done due to inheritance

The ones without already existing setup functions can be removed. Good.

Markus-Goetz · 2020-04-29T10:54:15Z

heat/core/tests/test_suites/basic_test.py

+            ht.use_device("gpu")
+            torch.cuda.set_device(torch.device(ht.gpu.torch_device))
+            torch_device = ht.cpu.torch_device
+            ht_device = ht.cpu


The set_device here is sufficient. I do not think that the explicit device passing, i.e. device=self.ht_device is necessary in the test cases anymore.

I use the ht_device for the special cases lcpu and lgpu where a tensor is created which is different from the standard device.

not passing the device directly does not work for me

do we really want to test this?

A user can still want to put a specific tensor on the cpu.
It tests that a function returns the a tensor of the input device type and not the use_device one.

But the opposite case in 'lgpu' is okay? It is setting gpu specifically while use_device is cpu?

Also not, there should be GPU or CPU only for now

Then why do we even offer the device parameter on tensor creation? 😕

Unless we are not meaning the same thing, allocating individual tensors on a different device should very much be allowed. For the test cases it is generally

a) not necessary to do it in this way,
b) should only be tested in a limited number of test cases in the factories function.

What I am getting here is, that you want to have different devices on different MPI ranks. This would go against the BSP model. If you want to write test cases that explicitly set the device on factory creation that is possible without the introduction of lcpu, lgpu. If you want to test whether you can allocate a DNDarray on a device different from the global one, there is no need for lcpu and lgpu, but rather something along the lines of

if ht.device == ht.cpu and torch.cuda.is_available: self.other_device = ht.gpu else: self.other_device = ht.cpu

As you are setting the torch device here as well. We can also remove all the self.device.torch_device calls from the test cases right?

heat/core/tests/test_suites/basic_test.py

Markus-Goetz · 2020-04-29T11:02:45Z

heat/core/tests/test_suites/basic_test.py

@@ -1,4 +1,5 @@
 from unittest import TestCase
+import os


This whole file still needs an init.py

init.py is no longer needed for Python 3.3+

Or what do you mean?

You are right, it is not needed anymore. However, we have the init.pys in all the folders. I think we should be consistent here.

…ytics/heat into enhancement/select-device

Markus-Goetz · 2020-06-16T08:32:41Z

heat/core/tests/test_suites/basic_test.py

+        return TestCase.__device
+
+    @classmethod
+    def setUpClass(cls):


Docsstring need to be updated here by the Raises section

Raises added

I found that there is already an __init__.py in the test_suites folder.

Pytorch doesn't have an option to set the standard device. It is always CPU. torch.cuda.set_device() only sets the default GPU if no ID is given

Can you import the test cases there as well?

Understood.

Markus-Goetz · 2020-06-16T08:38:13Z

heat/core/tests/test_suites/basic_test.py

@@ -1,4 +1,5 @@
 from unittest import TestCase
+import os


You are right, it is not needed anymore. However, we have the init.pys in all the folders. I think we should be consistent here.

Markus-Goetz · 2020-06-16T08:38:52Z

heat/core/tests/test_suites/basic_test.py

+            ht.use_device("gpu")
+            torch.cuda.set_device(torch.device(ht.gpu.torch_device))
+            torch_device = ht.cpu.torch_device
+            ht_device = ht.cpu


As you are setting the torch device here as well. We can also remove all the self.device.torch_device calls from the test cases right?

Markus-Goetz · 2020-06-16T12:23:08Z

Bum conflicts

mtar added 16 commits February 14, 2020 08:51

rewrite selection

042bea1

fix wrong skippping in test_device.py

590e765

use variable

1ae04f4

add missing references

65012ea

sort functions in distance.py following stylistic guideline

32a9ab5

revert

e3c3ed7

add missing device in distance.py

5832958

add second missing device

4d0c63a

update header

b6b9d5e

add missing devices

f75b80c

add header test_devices

8eea734

remove old code

8010878

fix assign new device

f1ae63f

fix missing heat device change on DNDarray.cpu

fc3b38e

Merge branch 'master' into enhancement/select-device

1227183

changelog

bd45727

mtar added the testing Implementation of tests, or test-related issues label Feb 17, 2020

Merge branch 'master' into enhancement/select-device

680ad9c

Markus-Goetz reviewed Feb 18, 2020

View reviewed changes

heat/core/tests/test_devices.py Outdated Show resolved Hide resolved

Markus-Goetz and others added 4 commits February 18, 2020 20:34

Merge branch 'master' into enhancement/select-device

7ac3282

refactor the 'device header' into a seperate module

df11692

formatting

fc02990

Merge branch 'master' into enhancement/select-device

a44f03b

coquelin77 requested changes Mar 21, 2020

View reviewed changes

change envar name & changelog

58ec0b1

mtar added 2 commits March 23, 2020 14:46

revert heat_device back to ht_device

335d955

move code to device.py

768246f

Merge branch 'master' into enhancement/select-device

2374ae8

Markus-Goetz requested changes Apr 29, 2020

View reviewed changes

mtar and others added 16 commits April 29, 2020 15:32

remove unneccessary functions

bac8ec6

rename BasicTest completely

ffd6e7b

Merge branch 'master' into enhancement/select-device

b355451

rename BasicTest to TestCase & remove local cases

36efc80

Merge branch 'enhancement/select-device' of github.com:helmholtz-anal…

c7c596e

…ytics/heat into enhancement/select-device

rename class members & update tests

a4ca8db

remove unsused variable

79925b5

update test_lasso

093ab69

Merge branch 'master' into enhancement/select-device

499cc1e

Merge branch 'master' into enhancement/select-device

1b178cd

Update CHANGELOG.md

1ffe420

remove device parameters in new tests

d9eac6c

Merge branch 'master' into enhancement/select-device

f269458

Merge branch 'master' into enhancement/select-device

6eb50f1

Merge branch 'master' into enhancement/select-device

4e98d25

Merge branch 'master' into enhancement/select-device

ff195ee

Markus-Goetz requested changes Jun 16, 2020

View reviewed changes

mtar and others added 4 commits June 16, 2020 12:01

add raises paragraph

2d94115

black

920b973

Merge branch 'master' into enhancement/select-device

6fb10f8

update torch_device

9799411

mtar added 3 commits June 16, 2020 15:03

Merge branch 'master' into enhancement/select-device

31f2438

black formatting

fbda362

doc

01eccf1

Markus-Goetz approved these changes Jun 16, 2020

View reviewed changes

Markus-Goetz merged commit 42bf442 into master Jun 16, 2020

Markus-Goetz deleted the enhancement/select-device branch June 16, 2020 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/select device #488

Enhancement/select device #488

mtar commented Feb 17, 2020 •

edited

codecov bot commented Feb 17, 2020 •

edited

coquelin77 left a comment

coquelin77 Mar 21, 2020

coquelin77 Mar 21, 2020

mtar Mar 23, 2020

coquelin77 Mar 23, 2020

mtar commented Mar 23, 2020

coquelin77 commented Mar 23, 2020

Markus-Goetz Apr 29, 2020

mtar Apr 29, 2020

Markus-Goetz Apr 30, 2020

Markus-Goetz Apr 29, 2020

mtar Apr 29, 2020

Markus-Goetz Apr 29, 2020

mtar Apr 29, 2020

mtar Apr 30, 2020

Markus-Goetz Apr 30, 2020

mtar Apr 30, 2020

mtar Apr 30, 2020 •

edited

Markus-Goetz Apr 30, 2020

mtar Apr 30, 2020

Markus-Goetz Apr 30, 2020

Markus-Goetz Jun 16, 2020

Markus-Goetz Apr 29, 2020

mtar Jun 3, 2020

mtar Jun 3, 2020

Markus-Goetz Jun 16, 2020

Markus-Goetz Jun 16, 2020

mtar Jun 16, 2020 •

edited

Markus-Goetz Jun 16, 2020

Markus-Goetz Jun 16, 2020

Markus-Goetz Jun 16, 2020

Markus-Goetz commented Jun 16, 2020

Enhancement/select device #488

Enhancement/select device #488

Conversation

mtar commented Feb 17, 2020 • edited

Description

Changes proposed:

Type of change

Due Diligence

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

codecov bot commented Feb 17, 2020 • edited

Codecov Report

coquelin77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtar commented Mar 23, 2020

coquelin77 commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtar Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtar Jun 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Markus-Goetz commented Jun 16, 2020

mtar commented Feb 17, 2020 •

edited

codecov bot commented Feb 17, 2020 •

edited

mtar Apr 30, 2020 •

edited

mtar Jun 16, 2020 •

edited