Dump cluster state on all test failures by crusaderky · Pull Request #5674 · dask/distributed

crusaderky · 2022-01-20T15:30:50Z

Change @gen_cluster to perform a state dump on all test failures (notably, including xfails - this should be negligible in terms of extra cost) instead of just timeouts.

Shorten the stack trace in case of @gen_cluster timeout.

gjoseph92

LGTM. I could see not wanting dumps from xfails in the future, but I assume you're thinking that at the moment, there could be useful information in there?

crusaderky · 2022-01-20T17:39:09Z

LGTM. I could see not wanting dumps from xfails in the future, but I assume you're thinking that at the moment, there could be useful information in there?

Not really - it's just not straightforward to detect xfails from where I'm dumping and there's very little if any harm done by dumping a few extra kilobytes

distributed/utils_test.py

Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>

jrbourbeau

Thanks @crusaderky! Will merge after CI finishes up

crusaderky · 2022-01-20T21:26:43Z

@jrbourbeau I'd like to verify if the trick to avoid dumping xfailed tests works first

jrbourbeau · 2022-01-20T21:32:29Z

Sure, sounds good. FWIW I ran distributed/tests/test_scheduler.py::test_non_existent_worker

distributed/distributed/tests/test_scheduler.py

Lines 1321 to 1335 in 7ebeda4

    
           @pytest.mark.xfail() 
        
           @gen_cluster(client=True, nthreads=[]) 
        
           async def test_non_existent_worker(c, s): 
        
               with dask.config.set({"distributed.comm.timeouts.connect": "100ms"}): 
        
                   await s.add_worker( 
        
                       address="127.0.0.1:5738", 
        
                       status="running", 
        
                       nthreads=2, 
        
                       nbytes={}, 
        
                       host_info={}, 
        
                   ) 
        
                   futures = c.map(inc, range(10)) 
        
                   await asyncio.sleep(0.300) 
        
                   assert not s.workers 
        
                   assert all(ts.state == "no-worker" for ts in s.tasks.values())

locally a test_cluster_dump/test_non_existent_worker.yaml file was created

crusaderky · 2022-01-20T22:27:19Z

Trying again

crusaderky · 2022-01-21T10:12:21Z

I've updated a wealth of tests that require a parameterized gen_cluster. One of them was xfailed and was simply outputting "test.yaml" as a dump. Please review again.

Known issues

If a parameterized test fails more than once, you'll only get the output from the last failure
The xfail marker is not detected when it's applied to individual parameters of @pytest.mark.parametrize

crusaderky · 2022-01-21T10:15:02Z

distributed/tests/test_client.py

-    test()
+    await wait(fut)
+    assert s.tasks
+    assert all(ts.annotations == {"foo": "bar"} for ts in s.tasks.values())


Fixed race condition where the test would potentially find an empty s.tasks dict

crusaderky · 2022-01-21T10:15:20Z

distributed/tests/test_scheduler.py

        config={"distributed.scheduler.work-stealing": False},
    )
-    async def test(c, s, *workers):
+    async def test_decide_worker_coschedule_order_neighbors_(c, s, *workers):


Ensure unique dump file name

crusaderky · 2022-01-21T10:15:42Z

distributed/tests/test_steal.py

 )
 def test_balance(inp, expected):
-    async def test(*args, **kwargs):
+    async def test_balance_(*args, **kwargs):


Ensure unique dump file name

crusaderky · 2022-01-21T10:16:17Z

distributed/utils_test.py

                worker["address"] = worker["queue"].get(timeout=5)
        except queue.Empty:
-            raise pytest.xfail.Exception("Worker failed to start in test")
+            pytest.xfail("Worker failed to start in test")


they're the same - this looks cleaner imho

crusaderky · 2022-01-21T12:26:27Z

Test output and artifacts inspected - ready for final review and merge

jrbourbeau · 2022-01-21T17:22:17Z

distributed/diagnostics/tests/test_worker_plugin.py

+        while "x" in a.tasks:
+            await asyncio.sleep(0.01)


I'm curious why this addition was needed

Because nothing tests for the existence of the release_key method in the plugin until a key is released

commit 14bddba1482de035ca416368f918d94b115d3660 Merge: df079fc 9a66b71 Author: crusaderky <crusaderky@gmail.com> Date: Tue Jan 25 17:16:58 2022 +0000 Merge branch 'main' into AMM/RetireWorker commit df079fc Merge: 9676934 5835ce3 Author: crusaderky <crusaderky@gmail.com> Date: Mon Jan 24 15:08:51 2022 +0000 Merge branch 'AMM/staging' into AMM/RetireWorker commit 5835ce3 Merge: ee68b02 b0b8e95 Author: crusaderky <crusaderky@gmail.com> Date: Mon Jan 24 15:07:27 2022 +0000 Merge branch 'AMM/test_close_gracefully' into AMM/staging commit ee68b02 Merge: 7d4e6ee ccad288 Author: crusaderky <crusaderky@gmail.com> Date: Mon Jan 24 15:06:50 2022 +0000 Merge branch 'main' into AMM/staging commit b0b8e95 Author: crusaderky <crusaderky@gmail.com> Date: Mon Jan 24 14:32:29 2022 +0000 Code review commit 9676934 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 14:50:53 2022 +0000 AMM to manage retire_workers() commit 7d4e6ee Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 14:39:41 2022 +0000 Fix flaky test_close_gracefully and test_lifetime (dask#5677) commit 7faab51 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 14:39:41 2022 +0000 harden test commit aef3b71 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 14:29:10 2022 +0000 Increase resilience on slow CI commit af84e40 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:39:18 2022 +0000 Dump cluster state on all test failures (dask#5674) commit 5054c19 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:38:38 2022 +0000 Paused workers shouldn't steal tasks (dask#5665) commit eadb35f Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:37:48 2022 +0000 transition flight to missing if no who_has (dask#5653) commit 581aee8 Merge: 940bb45 60c0d60 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:36:09 2022 +0000 Merge branch 'main' into AMM/test_close_gracefully commit 940bb45 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:20:10 2022 +0000 tweak comment commit 731d132 Author: crusaderky <crusaderky@gmail.com> Date: Fri Jan 21 12:12:03 2022 +0000 Fix flaky test_close_gracefully and test_lifetime

Dump cluster state on all test failures

52a9e74

crusaderky self-assigned this Jan 20, 2022

crusaderky mentioned this pull request Jan 20, 2022

AMM: Graceful Worker Retirement #5381

Merged

gjoseph92 self-requested a review January 20, 2022 17:04

gjoseph92 approved these changes Jan 20, 2022

View reviewed changes

gjoseph92 reviewed Jan 20, 2022

View reviewed changes

distributed/utils_test.py Outdated Show resolved Hide resolved

crusaderky and others added 2 commits January 20, 2022 18:38

Update distributed/utils_test.py

4ec9331

Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>

lint

caa7542

gjoseph92 approved these changes Jan 20, 2022

View reviewed changes

jrbourbeau approved these changes Jan 20, 2022

View reviewed changes

Detect xfail

597b29f

Overhaul tests with parametrized gen_cluster

ef77f08

crusaderky commented Jan 21, 2022

View reviewed changes

crusaderky added a commit to crusaderky/distributed that referenced this pull request Jan 21, 2022

Dump cluster state on all test failures (dask#5674)

af84e40

gjoseph92 approved these changes Jan 21, 2022

View reviewed changes

jrbourbeau reviewed Jan 21, 2022

View reviewed changes

crusaderky merged commit 0595052 into dask:main Jan 21, 2022

crusaderky deleted the dump_on_all_errors branch January 21, 2022 17:25

ian-r-rose mentioned this pull request Jan 26, 2022

Check for new name of timeouts artifact and be more fault tolerant. #5707

Merged

2 tasks

Uh oh!

Conversation

crusaderky commented Jan 20, 2022

Uh oh!

gjoseph92 left a comment

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Jan 20, 2022

Uh oh!

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Jan 20, 2022

Uh oh!

jrbourbeau commented Jan 20, 2022

Uh oh!

crusaderky commented Jan 20, 2022

Uh oh!

crusaderky commented Jan 21, 2022

Known issues

Uh oh!

crusaderky Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Jan 21, 2022

Uh oh!

jrbourbeau Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants