Test P2P Shuffle in integration tests #597

hendrikmakait · 2022-12-07T13:23:59Z

Closes Test P2P Shuffle in integration tests #569

conftest.py

hendrikmakait · 2022-12-23T10:12:23Z

Skipping larger data size for join tests due to data transfer to client which OOM-kills the pytest workers (#633).

hendrikmakait · 2022-12-23T11:27:34Z

Ready for review. cc @crusaderky, @ncclementi

hendrikmakait · 2022-12-23T11:28:05Z

tests/benchmarks/test_join.py

+    (0.1, "tasks"),
+    # shuffling takes a long time with 1 or higher
+    (0.1, "p2p"),
+    # (1, "p2p"),


#633 should allow us to enable this in a follow-up

Shouldn't we mark them then as marks=pytest.mark.skip(reason="Does not finish, fix when #633 is merged")

If not, open an issue to state that when the #633 is merged we need to uncomment this

ncclementi

Left some small comments.

My only concern is that we will be running each of the h2o queries 9 times (csv, pq0.5GB, pq5GB * no_shuffle, task, p2p) This will increase the time of the whole CI, a lot. For most of the queries and this size of data shuffle and p2p might make no sense.

Shouldn't we just move to only 1 biggish pq dataset (thinking of the 50GB) and run this only maybe once a week. What would imply running it as a separate workflow probably.

tests/benchmarks/test_join.py

ncclementi · 2022-12-23T15:04:58Z

tests/benchmarks/test_join.py

+    (0.1, "tasks"),
+    # shuffling takes a long time with 1 or higher
+    (0.1, "p2p"),
+    # (1, "p2p"),


Shouldn't we mark them then as marks=pytest.mark.skip(reason="Does not finish, fix when #633 is merged")

If not, open an issue to state that when the #633 is merged we need to uncomment this

Co-authored-by: Naty Clementi <[email protected]>

hendrikmakait · 2022-12-23T15:52:46Z

My only concern is that we will be running each of the h2o queries 9 times (csv, pq0.5GB, pq5GB * no_shuffle, task, p2p) This will increase the time of the whole CI, a lot. For most of the queries and this size of data shuffle and p2p might make no sense.

FWIW, we only run each query 6 times as we do not test the implicit default aka "no_shuffle". IIUC, tasks is the default behavior when using a distributed cluster, we just make it explicit now and do not need to test "no_shuffle". While there shouldn't be a difference in performance, we want to avoid possible performance regressions. Regarding frequency/increased CI runtime, I've had a chat with @fjetter about this and while tasks is the default behavior and p2p is under active development, we feel like it would be a good idea to test both of them frequently. Note that until the next release, p2p is only tested for upstream.

Shouldn't we just move to only 1 biggish pq dataset (thinking of the 50GB) and run this only maybe once a week. What would imply running it as a separate workflow probably.

Adding testing on a large-scale dataset is yet another thing we should add, but that's out of scope for this PR.

hendrikmakait · 2022-12-23T15:53:43Z

Shouldn't we mark them then as marks=pytest.mark.skip(reason="Does not finish, fix when #633 is merged")

Good point, I meant to do that after creating #633.

hendrikmakait · 2022-12-23T16:09:12Z

tests/benchmarks/test_join.py

    pytest.param(
-        1,
-        marks=pytest.mark.skip(reason="Does not finish"),
+        (1, "p2p"), marks=pytest.mark.skip(reason="client OOMs, see coiled-runtime#633")


hendrikmakait · 2022-12-23T16:09:25Z

tests/benchmarks/test_join.py

-        10,
-        marks=pytest.mark.skip(reason="Does not finish"),
+        (10, "p2p"),
+        marks=pytest.mark.skip(reason="client OOMs, see coiled-runtime#633"),


hendrikmakait · 2022-12-23T16:40:59Z

Running the H2O benchmarks with p2p took me a total of 13 mins. 7 mins were spent on cluster setup and testing 0.5 GB datasets. With that in mind, I do not see any harm in testing p2p on every run.

Granted, the value of testing on tiny datasets is questionable, but should be discussed in another issue.

ncclementi · 2022-12-23T18:21:10Z

@hendrikmakait We are having some legit failures in https://github.com/coiled/coiled-runtime/actions/runs/3766959977/jobs/6404088081

________________ ERROR collecting tests/benchmarks/test_join.py ________________
tests/benchmarks/test_join.py::test_join_big: in "parametrize" the number of names (2):
  ['mem_mult', 'shuffle']
must be equal to the number of values (1):
  ((1, 'p2p'),)

The ones in https://github.com/coiled/coiled-runtime/actions/runs/3766959977/jobs/6404088006 are platform related.

ncclementi · 2022-12-23T19:11:18Z

@hendrikmakait I'm trying different combinations here #634, but haven't been able to make it work.

That PR, runs only that test so it's easier to debug. in case you want to mess in there.
I tried, this but didn't work

  (
      pytest.param(1, marks=pytest.mark.skip(reason=reason)),
      pytest.param("p2p", marks=pytest.mark.skip(reason=reason)),
  )

hendrikmakait · 2022-12-23T19:17:40Z

@ncclementi: I think I found the solution (interesting that some stuff seems to work but then fails when it comes to running the specific parametrization (or ignoring it).

Sorry for causing this, I should've really tested "that one last beautification" locally.

ncclementi · 2022-12-23T19:20:52Z

@hendrikmakait no worries, I've been playing with it trying to fix it because I thought you were off for today. I'm testing it over here too as it should fail faster if it doesn't work #634

It's very unintuitive :/

ncclementi · 2022-12-23T23:44:27Z

Ci is green,
Thanks @hendrikmakait this goes in!

hendrikmakait added 4 commits November 30, 2022 16:36

Parametrize shuffle type for shuffle tests

02207c9

Skip if P2P not available

6d8dbfb

P2P in join

85a1d2f

Minor

d172a53

ncclementi reviewed Dec 7, 2022

View reviewed changes

conftest.py Outdated Show resolved Hide resolved

hendrikmakait added 20 commits December 7, 2022 16:46

Parametrize h2o

6dba930

h2o

afcf657

Use client fixture

2d0c3a7

Merge branch 'main' into shuffle-fixture

ef91c70

minor

d62c422

Pin packaging (to be removed)

f449000

Merge branch 'main' into shuffle-fixture

53403c6

Merge branch 'main' into shuffle-fixture

b6c8ef5

Adjust fixture setup

f27c37b

Remove packaging restriction again

7cc5518

Run larger join tests

d5b6d02

Merge branch 'main' into shuffle-fixture

d616029

Remove large joins

63ace5b

line break

c44dff0

Adjust p2p min version

074a347

Rename tests in DB

9a5840d

Parametrize df.shuffle test

ddcc796

Adjust migration

3acc67d

Typo

6c9588c

Avoid transformation issue

482388e

hendrikmakait marked this pull request as ready for review December 23, 2022 10:12

hendrikmakait commented Dec 23, 2022

View reviewed changes

ncclementi reviewed Dec 23, 2022

View reviewed changes

Update tests/benchmarks/test_join.py

c13cb6a

Co-authored-by: Naty Clementi <[email protected]>

Properly skip large data

393da08

hendrikmakait commented Dec 23, 2022

View reviewed changes

Proper P2P_AVAILABLE

c4bedd9

hendrikmakait added 2 commits December 23, 2022 20:06

Fix params

61b9001

ordering

ee884da

Minor

b9880e7

ncclementi approved these changes Dec 23, 2022

View reviewed changes

ncclementi merged commit ce2ea4f into main Dec 23, 2022

ncclementi deleted the shuffle-fixture branch December 23, 2022 23:44

hendrikmakait mentioned this pull request Jan 31, 2023

Remove storing memory samples to CSV #678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test P2P Shuffle in integration tests #597

Test P2P Shuffle in integration tests #597

hendrikmakait commented Dec 7, 2022 •

edited

Loading

hendrikmakait commented Dec 23, 2022

hendrikmakait commented Dec 23, 2022

hendrikmakait Dec 23, 2022

ncclementi Dec 23, 2022

ncclementi left a comment

ncclementi Dec 23, 2022

hendrikmakait commented Dec 23, 2022

hendrikmakait commented Dec 23, 2022

hendrikmakait Dec 23, 2022

hendrikmakait Dec 23, 2022

hendrikmakait commented Dec 23, 2022

ncclementi commented Dec 23, 2022

ncclementi commented Dec 23, 2022 •

edited

Loading

hendrikmakait commented Dec 23, 2022

ncclementi commented Dec 23, 2022

ncclementi commented Dec 23, 2022

Test P2P Shuffle in integration tests #597

Test P2P Shuffle in integration tests #597

Conversation

hendrikmakait commented Dec 7, 2022 • edited Loading

hendrikmakait commented Dec 23, 2022

hendrikmakait commented Dec 23, 2022

hendrikmakait Dec 23, 2022

Choose a reason for hiding this comment

ncclementi Dec 23, 2022

Choose a reason for hiding this comment

ncclementi left a comment

Choose a reason for hiding this comment

ncclementi Dec 23, 2022

Choose a reason for hiding this comment

hendrikmakait commented Dec 23, 2022

hendrikmakait commented Dec 23, 2022

hendrikmakait Dec 23, 2022

Choose a reason for hiding this comment

hendrikmakait Dec 23, 2022

Choose a reason for hiding this comment

hendrikmakait commented Dec 23, 2022

ncclementi commented Dec 23, 2022

ncclementi commented Dec 23, 2022 • edited Loading

hendrikmakait commented Dec 23, 2022

ncclementi commented Dec 23, 2022

ncclementi commented Dec 23, 2022

hendrikmakait commented Dec 7, 2022 •

edited

Loading

ncclementi commented Dec 23, 2022 •

edited

Loading