Autogluon: Scheduler/searcher runs infinitely long (fails to handle error in individual training trials)

[jwmueller]

HPO may currently run infinitely (due to deadlock) when any individual training trial has an error.

May be root cause of issues like this: https://github.com/awslabs/autogluon/issues/245

A temporary fix is implemented for TabularPrediction in: https://github.com/awslabs/autogluon/pull/322/

But a more long-term fix should live in scheduler/searcher.

[zhanghang1989]

Thanks @jwmueller

As we discussed, this need to be implemented in the scheduler to handle the reporter when exception happens in the user/pipeline code.

We can prioritize it when we refactor searchers & schedulers with new algorithms added.

[mseeger]

Hello,

this is something I am interested in. We will add new searchers that come with a certain complexity, and they may well fail with throwing an exception (even though this happens very rarely for us).

We'd love that in this case, the whole experiment is shut down.

In the longer term, we should consider alternatives. Maybe the very simplest is to catch exceptions and return some "invalid evaluation" code. The searcher/scheduler would then have the chance to react to this, for example they could just ignore the evaluation call (as there is quite some randomness involved, this may well help to recover).

We also have HPO variants which can deal with failures systematically. This is an exciting and useful direction.

[Innixma]

Why not have a parameter to Searcher which dictates behavior upon receiving an exception: Shutdown, ignore+continue, crash, etc.?

[Innixma]

Additionally, I'd like to comment that skopt Searcher randomly deadlocks even after #322 has been applied. In 3 runs, 2 of them deadlocked at different points and 1 succeeded. It's unknown why it does this. (Tested on MacOS, LightGBM and CatBoost models)

When interrupting, I got this stack trace consistently:

Traceback (most recent call last):
  File "/anaconda3/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/anaconda3/lib/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/neerick/workspace/awslabs/autogluon/autogluon/scheduler/scheduler.py", line 138, in _worker
    ret = fn(**args)
  File "/Users/neerick/workspace/awslabs/autogluon/autogluon/core/decorator.py", line 51, in __call__
    self._rand_seed()
  File "/Users/neerick/workspace/awslabs/autogluon/autogluon/core/decorator.py", line 115, in _rand_seed
    np.random.seed(_autogluon_method.SEED.value)
  File "mtrand.pyx", line 163, in numpy.random.mtrand.RandomState.seed
  File "mt19937.pyx", line 167, in numpy.random.mt19937.MT19937._legacy_seeding
KeyboardInterrupt

[jwmueller]

It seems the base RandomSearcher was updated with some locking functionality that was never added to the skopt searcher: https://github.com/awslabs/autogluon/blob/master/autogluon/searcher/searcher.py#L162

@zhanghang1989 Assuming you understand this locking functionality the best, can you add the same functionality to the skopt searcher?
In general, we really need to ensure that whoever alters any of the searchers/schedulers makes sure that the same changes are applied to all the other searchers/schedulers (where appropriate).

[mseeger]

I may have put in the additional locking. While I lack the understanding of Hang, I'd rather err on the cautious side. Louis once told me that both read and write access to class members should be protected. If members are objects with methods, then maybe they should have their own locks. At least, this seems to me how the code works.

[mseeger]

@Innixma We could ask autogluon evaluation functions to return a certain status, but this sounds cumbersome.

Maybe better (if feasible) would be to wrap any such evaluation call into try catch, and if something is caught, we return a "failed" signal and maybe the exception object?

Then, it would be up to the scheduler and searcher to react to a failed evaluation, which is probably the best solution. As long as we do not want to implement this, the default reaction of a scheduler could be to tear down all running jobs and rethrow the exception?

[Innixma]

@mseeger What I think is ideal is that if an exception occurs in a trial started by a scheduler, it should receive the exception and handle it in a manner which does not result in deadlock and is well explained. By giving a new parameter to scheduler, we could dictate what this behavior is (Ignore and continue, raise exception, stop HPO early, etc.)

Currently, when a trial has an exception, the entire scheduler deadlocks. This is what has to be changed. I am not familiar with how to accomplish this, but I'm sure it can be done in relatively few lines of code by those familiar with the schedulers.

[Innixma]

I may have put in the additional locking. While I lack the understanding of Hang, I'd rather err on the cautious side. Louis once told me that both read and write access to class members should be protected. If members are objects with methods, then maybe they should have their own locks. At least, this seems to me how the code works.

Regarding this, could you apply the same additional locks to skopt searcher? Currently it is unstable.

[jwmueller]

Perhaps we can better utilize inheritance / class hierarchy to prevent issues like this going forward?

For example, in the BaseSearcher, we could have something like this:

def get_config(self, **kwargs):
   with self.LOCK:
      if len(self._results) == 0:  # no hyperparams have been tried yet, first try default config
            return self.default_config()
      else:
            try: 
                return self._get_config()
            except: 
                ErrorHandling

def _get_config(self, **kwargs):
    raise NotImplementedError

and then instead of get_config() method, each usable Searcher can have internal _get_config() method used to return the next configuration, which doesn't have to worry about error-handling, locking, etc. We can probably do something similar for updating rewards, etc.

[mseeger]

Hmm, I think get_config is the wrong one to wrap. Recall that there are two things happening:

1) Scheduler obtains new value(s) from jobs via reporter, calls update of searcher
2) Scheduler asks searcher for new config to start (get_config)

When a job fails, this should go through 1), not 2). What would be great if the reporter could be asked whether the job has failed. An easy idea would be that this could be returned in fetch.
This is easy, because the scheduler calls fetch anyway regularly in order to pull new metric values.

Then, the scheduler can react to it. If something like this was done by somebody, I could rewrite the scheduler(s) and searchers, at least to react in one defined way.

But I am not able to do this stuff for reporter and autogluon function annotation. Ideally, we want to have something that wraps the evaluaiton function provided by the user and catches exceptions. I'd not know how to do that.

[razmikmelikbekyan]

Hello, I experimented similar problems, when using schedulers for training some custom train functions. I suggest the following solution.

• to implement custom process object, which provides info about exception

• before trying to get info from local reporter queue we can check the exception of the process (which is the process which is running model trial)
  https://github.com/awslabs/autogluon/blob/5d4ea45541d1c9c42b3720a5fed29f12cb81364a/autogluon/scheduler/reporter.py#L178

• if there is an exception terminate the reporter and so on

import multiprocessing as mp
import traceback

class Process(mp.Process):
    def __init__(self, *args, **kwargs):
        mp.Process.__init__(self, *args, **kwargs)
        self._pconn, self._cconn = mp.Pipe()
        self._exception = None

    def run(self):
        try:
            mp.Process.run(self)
            self._cconn.send(None)
        except Exception as e:
            tb = traceback.format_exc()
            self._cconn.send((e, tb))
            # raise e  # You can still rise this exception if you need to

    @property
    def exception(self):
        if self._pconn.poll():
            self._exception = self._pconn.recv()
        return self._exception

Example:

def target():
    raise ValueError('Something went wrong...')

p = Process(target = target)
p.start()
p.join()

if p.exception:
    error, traceback = p.exception
    print(traceback)

The solution is from here: https://stackoverflow.com/questions/19924104/python-multiprocessing-handling-child-errors-in-parent

[jwmueller]

Hmm, I think get_config is the wrong one to wrap. Recall that there are two things happening:

1. Scheduler obtains new value(s) from jobs via reporter, calls update of searcher
2. Scheduler asks searcher for new config to start (get_config)

When a job fails, this should go through 1), not 2). What would be great if the reporter could be asked whether the job has failed. An easy idea would be that this could be returned in fetch.
This is easy, because the scheduler calls fetch anyway regularly in order to pull new metric values.

Then, the scheduler can react to it. If something like this was done by somebody, I could rewrite the scheduler(s) and searchers, at least to react in one defined way.

But I am not able to do this stuff for reporter and autogluon function annotation. Ideally, we want to have something that wraps the evaluaiton function provided by the user and catches exceptions. I'd not know how to do that.

My pseudocode / suggestion to better utilize inheritance was not really intended to address error-handling specifically. Rather it is to address an issue I keep seeing: as new changes are being made to improve certain schedulers/searchers, they are not being made in the rest of the searchers/schedulers thus leading to inconsistencies that shouldn't exist (and this'll get worse as more algorithms are added). My suggestion is only this: whenever introducing a change to an existing searcher/scheduler, consider whether that change would be more appropriate in the Base class instead of just the particular class you're currently working on.

Some example inconsistencies that currently exist and should be addressed include:

• locking in some searchers and not others
• using default_config in HPO round #1 should always happen regardless of searcher (this is critical for success in time-constrained settings)
• error-handling is not standardized

My psuedocode was only trying to demonstrate that we can easily resolve the first 2 of these inconsistencies. Presumably there is also a homogeneous way to deal with error-handling in the base class.

[mseeger]

@jwmueller
You are making great points. It would be great to formulate the requirements for a new searcher in the header comments of BaseSearcher. Even better if they were enforced by abstract methods.

I'd be very interested in this, because as you know, we are about to contribute a bunch of new searchers which are quite sophisticated, and I am not sure we are currently supporting all the points you mention.

As for locking, I'd suggest @zhanghang1989 is looking over the code, or otherwise tells us what to lock. My intuition is that I want to wrap every read/write access to a member in a lock. The exception is if the member is itself an object, and the access happens through a method, in which case locking is deferred to that class. Is that correct? I'd be surprised if not.

My feeling is locks are simple (at least conservative), I do have some understanding problems with semaphores though ...

[mseeger]

I will make sure our new searchers respect the default config requirement.

[mseeger]

OK, while I am doing this: Are you sure that configspace.get_default_configuration() always returns a sensible config to evaluate at? What I mean, is the AG user who specifies the evaluation function, also required to provide a default config?

[jwmueller]

OK, while I am doing this: Are you sure that configspace.get_default_configuration() always returns a sensible config to evaluate at? What I mean, is the AG user who specifies the evaluation function, also required to provide a default config?

Regarding this point, I think all of our default HPO spaces (used when the user does not specify anything) should absolutely have a well thought-out default_config. This is one of the easiest ways to improve performance reliability. For user-defined custom HPO spaces, I believe ConfigSpace already automatically selects defaults even when they are not specified by the creator of the HPO space (eg. midpoint for numeric intervals: https://automl.github.io/ConfigSpace/master/API-Doc.html#integer-hyperparameters). It seems to me even when these custom defaults are unknown to the user, they will still typically be >= a random config. So there's never any onus on users to have to think of good defaults.

I'll let @zhanghang1989 address the issues around locking.

[Innixma]

This has now been resolved thanks to #345 and #354. Resolving the issue.