is_alive doctest failure in map_reduce

[videlec]

Some patchbots report unstopped workers

sage -t --long src/sage/parallel/map_reduce.py
**********************************************************************
File "src/sage/parallel/map_reduce.py", line 1090, in sage.parallel.map_reduce.RESetMapReduce.start_workers
Failed example:
    all(w.is_alive() for w in S._workers)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of   9 in sage.parallel.map_reduce.RESetMapReduce.start_workers
    [296 tests, 1 failure, 28.70 s]
----------------------------------------------------------------------
sage -t --long src/sage/parallel/map_reduce.py  # 1 doctest failed
----------------------------------------------------------------------

see

• this sardonis log

CC: @hivert

Component: combinatorics

Keywords: random_fail

Author: Florent Hivert

Branch/Commit: 6eeda41

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24241

[embray]

comment:2

I'm not sure an obvious way to reproduce this, but maybe we could go ahead and merge #21233 and see if that fixes it? I've been waiting forever for someone to just give it positive review (which it had previously but Volker removed it...)

[videlec]

comment:3

Replying to @embray:

I'm not sure an obvious way to reproduce this, but maybe we could go ahead and merge #21233 and see if that fixes it?

+1. And I would love to have a way to grep through the patchbot reports easily!

[embray]

comment:4

Replying to @videlec:

Replying to @embray:

I'm not sure an obvious way to reproduce this, but maybe we could go ahead and merge #21233 and see if that fixes it?

+1. And I would love to have a way to grep through the patchbot reports easily!

Open an issue on the patchbot GitHub project for that. I would love that too but it's probably not entirely trivial (if nothing else we'd want to index the report logs).

[embray]

comment:5

I'm not totally sure this was fixed by #21233. Now, on several of my Cygwin patchbot runs, this module fails on the initial test run, not quite in the way reported by this ticket, but possibly similar. I get sage -t --long src/sage/parallel/map_reduce.py # Timed out after testing finished which is something I've never seen before...

[vbraun]

comment:6

I'm also seeing this on the buildbot

[vbraun]

Changed keywords from none to random_fail

[jdemeyer]

comment:7

This is just to say that I got this again.

[jdemeyer]

comment:9

The doctest looks like a race condition. If I'm understanding things correctly, the workers are started and will then stop naturally (after an unspecified amount of time). If they stop really quickly, then this doctest will fail:

            sage: from sage.parallel.map_reduce import RESetMapReduce
            sage: S = RESetMapReduce(roots=[])
            sage: S.setup_workers(2)
            sage: S.start_workers()
            sage: all(w.is_alive() for w in S._workers)
            True

[jdemeyer]

comment:10

I can make this test fail pretty consistently with

sage: sleep(0.02); all(w.is_alive() for w in S._workers)

If a doctest is sensitive to 20ms delays, it's a bad test.

[embray]

comment:11

Indeed; I see the problem here. When I originally commented on this ticket, I admit, I don't think I looked very closely at the exact test that was failing.

If there's no work for the workers to do, then there's no guarantee that you'll ever find them all running simultaneously.

If you really wanted to test this, one possibility might be to set up a test logger that collects all log messages in a list, and then checks that the expected log messages are found (e.g. one "Started" and one "Exiting" for each worker started.

[hivert]

comment:12

Replying to @embray:

If you really wanted to test this, one possibility might be to set up a test logger that collects all log messages in a list, and then checks that the expected log messages are found (e.g. one "Started" and one "Exiting" for each worker started.

Thanks to all of you for catching this one. I'm confirming jdemeyer analysis. If there is no work to do, there is no robust lower bound for the time the worker stays alive.

@embray: there is a logger is the code but the level is normally set too low to see the message. Another possibilities would be to give as work to the worker a sleep(1) instruction.

[hivert]

Branch: u/hivert/is_alive_doctest_failure_in_map_reduce

[hivert]

comment:13

Sorry based my file on the wrong branch... Fixing it

[jdemeyer]

comment:14

The doctest fix looks good on first sight, I would still keep the sleep(1) in the last test though.

I cannot really comment on the other changes, which seem to be related to Python 3.

---

New commits:

8778e24

Tentative fix of MapReduce.is_alive

[jdemeyer]

Commit: 8778e24

[sagetrac-git]

Changed commit from 8778e24 to 9c1ab33

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9c1ab33

Fixed wrong base

[hivert]

comment:16

Replying to @jdemeyer:

The doctest fix looks good on first sight, I would still keep the sleep(1) in the last test though.

I cannot really comment on the other changes, which seem to be related to Python 3.

Sorry based my file on the wrong branch... Should be fixed now

---

New commits:

9c1ab33

Fixed wrong base

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

6eeda41

Put back timeout to 1

[sagetrac-git]

Changed commit from 9c1ab33 to 6eeda41

[hivert]

Author: Florent Hivert

[jdemeyer]

Reviewer: Jeroen Demeyer

[hivert]

comment:20

Replying to @jdemeyer:
Thanks Jeroen

[vbraun]

Changed branch from u/hivert/is_alive_doctest_failure_in_map_reduce to 6eeda41