Catch BaseException in host discovery thread to prevent silently dying

[ASDen]

When any error other than a RuntimeError occurs, the host discovery thread silently dies, and the training process is unaware of any changes in the list of hosts.

Fix this by catching any BaseException instead.

[maxhgerlach]

HI @ASDen,

thanks for you contribution! Note that you would need to sign the Developer Certificate of Origin by signing off your commit.

I'm not very familiar with the elastic code base (@tgaddair might know more about the error handling here), but BaseException feels awfully broad.

[maxhgerlach]

Wouldn't it be better to catch Exception here, rather than BaseException?

BaseException would also include SystemExit and KeyboardInterrupt and prevent the Python interpreter from shutting down. (Going by https://docs.python.org/3/library/exceptions.html#exception-hierarchy)

[EnricoMi]

I think RuntimeError here is ok, as it represents a retry-able exception. I agree that other exceptions should not kill the discovery thread. Instead of broadening the exception here, I would add a broader exception that always calls self._shutdown.set() to gracefully shutdown the Horovod job.

Then, the host manager and _notify_workers_host_changes should only raise RuntimeError for transient errors or some other exception for fatal errors.

I'd be more interested in the particular error that killed the discovery thread on your side (it must be in the logs).

[ASDen]

Thanks alot for having a look !

@maxhgerlach This shouldn't prevent the interpreter from shutting down, since this thread is marked as daemon here. However, I think Exception should also work.

@EnricoMi I think that in our case here, given that we have the host discovery thread repeatedly executing an arbitrary (possibly user defined) host discovery function that can have any random glitch for a few executions cycles then returns to normal, it makes a lot of sense to just log the error (whatever happened) to user, and continue normal

I would disagree with shutting down the whole training session, for an error (whatever its source) in the host discovery function.

Also, RuntimeError is far from being the only retry-able exception, maybe Exception mentioned by @maxhgerlach. But there are many retry-able exceptions that are not RuntimeError (e.g. ZeroDivisionError, FileNotFoundError, TimeoutError ,...etc)

I was personally hit by this in the form of a ImportError (inside a custom discovery class) that silently killed the host discovery thread (and it was awful to debug :(

[EnricoMi]

Agreed.

[maxhgerlach]

sounds as if Exception vs BaseException wouldn't make much of a difference here, so no objections from me

[ASDen]

Hi @EnricoMi @maxhgerlach any updates on this ?
I have another bug fix awaiting review here #3437, I have also another couple Elastic bug fixes that I would like to submit if maintainers are keen to have a look

Thanks,

[EnricoMi]

@ASDen please see the DCO issue and follow these instructions: https://github.com/horovod/horovod/pull/3436/checks?check_run_id=5376922918

[github-actions]

Unit Test Results

     802 files   -   28       802 suites   - 28   9h 48m 41s ⏱️ - 10m 10s
     757 tests ±    0       714 ✔️ ±    0       43 💤 ±    0  0 ❌ ±0 
18 684 runs   - 732  13 401 ✔️  - 468  5 283 💤  - 264  0 ❌ ±0 

Results for commit c9ee58a. ± Comparison against base commit 980ce05.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Results (with flaky tests)

     932 files  +  12       932 suites  +12   10h 31m 10s ⏱️ + 6m 55s
     757 tests ±    0       712 ✔️  -     1       43 💤 ±  0  1 ❌ ±0  1 🔥 +1 
21 822 runs  +270  15 395 ✔️ +240  6 425 💤 +29  1 ❌ ±0  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit c9ee58a. ± Comparison against base commit 980ce05.

♻️ This comment has been updated with latest results.

[ASDen]

@EnricoMi Done!

[maxhgerlach]

I finally clicked that merge button (assuming that that test failure was caused by infrastructure flakiness). Sorry for the delay, @ASDen.