Repository: optuna/optuna
Pull request: Record all error raising trials as failures
Fixes https://github.com/pfnet/optuna/issues/386.

Problem statement:
`study.optimize` should record any exceptional trial as `FAIL` even if the exception was not specified in `catch`.
**Conditions**
- Optuna version: 0.9.0
- Python version: 3.6.7
- OS: MacOS Mojave, 10.14.4
- Machine Learning library to be optimized: any

**Code to reproduce**

```
import optuna

def objective(trial):
    raise ValueError('no!!!')

if __name__ == '__main__':
    study = optuna.create_study()
    study.optimize(objective, n_trials=3, catch=())
```

**Error messages, stack traces, or logs**

```
Traceback (most recent call last):
  File "opt.py", line 8, in <module>
    study.optimize(objective, n_trials=3, catch=())
  File "/Users/keisuke.nakata/.pyenv/versions/3.6.7/lib/python3.6/site-packages/optuna/study.py", line 210, in optimize
    self._optimize_sequential(func, n_trials, timeout, catch)
  File "/Users/keisuke.nakata/.pyenv/versions/3.6.7/lib/python3.6/site-packages/optuna/study.py", line 324, in _optimize_sequential
    self._run_trial(func, catch)
  File "/Users/keisuke.nakata/.pyenv/versions/3.6.7/lib/python3.6/site-packages/optuna/study.py", line 398, in _run_trial
    result = func(trial)
  File "opt.py", line 4, in objective
    raise ValueError('no!!!')
ValueError: no!!!
```

I specified `catch=()` in order to detect all exceptions which will be raised in `objective`.
But in this case, optuna can't record exceptional trials with status `FAIL`, because `objective` itself ignores the exception at all.

So `objective` should record *any* exceptional trial as `FAIL`, even if the exception was not specified in `catch`.

I think it might better to have argument `reraise=True/False` in `optimize`, which controls `optimize` itself raises exceptions from `objective`.

Hints from issue discussion:

Commit message: Fix ChainerMN test

Patch to review:
diff --git a/optuna/study.py b/optuna/study.py
index 88e5d165b4d..cf791ba7acd 100644
--- a/optuna/study.py
+++ b/optuna/study.py
@@ -510,13 +510,16 @@ def _run_trial(self, func, catch):
             self.logger.info(message)
             self._storage.set_trial_state(trial_id, structs.TrialState.PRUNED)
             return trial
-        except catch as e:
+        except Exception as e:
             message = 'Setting status of trial#{} as {} because of the following error: {}'\
                 .format(trial_number, structs.TrialState.FAIL, repr(e))
             self.logger.warning(message, exc_info=True)
             self._storage.set_trial_system_attr(trial_id, 'fail_reason', message)
             self._storage.set_trial_state(trial_id, structs.TrialState.FAIL)
-            return trial
+
+            if any(isinstance(e, cls) for cls in catch):
+                return trial
+            raise
         finally:
             # The following line mitigates memory problems that can be occurred in some
             # environments (e.g., services that use computing containers such as CircleCI).
diff --git a/tests/integration_tests/test_chainermn.py b/tests/integration_tests/test_chainermn.py
index 3d141e0fca6..54c6928a353 100644
--- a/tests/integration_tests/test_chainermn.py
+++ b/tests/integration_tests/test_chainermn.py
@@ -248,9 +248,9 @@ def objective(_trial, _comm):
             # Assert trial count.
             assert len(mn_study.trials) == n_trials + 1
 
-            # Assert aborted trial count.
-            aborted_trials = [t for t in mn_study.trials if t.state == TrialState.RUNNING]
-            assert len(aborted_trials) == 1
+            # Assert failed trial count.
+            failed_trials = [t for t in mn_study.trials if t.state == TrialState.FAIL]
+            assert len(failed_trials) == n_trials + 1
 
     @staticmethod
     @pytest.mark.parametrize('storage_mode', STORAGE_MODES)
diff --git a/tests/test_study.py b/tests/test_study.py
index e375856c02b..a681491c7c5 100644
--- a/tests/test_study.py
+++ b/tests/test_study.py
@@ -232,10 +232,14 @@ def func_value_error(_):
 
         # Test acceptable exception.
         study.optimize(func_value_error, n_trials=20, catch=(ValueError, ))
+        assert len(study.trials) == 20
+        assert all(trial.state == optuna.structs.TrialState.FAIL for trial in study.trials)
 
         # Test trial with unacceptable exception.
         with pytest.raises(ValueError):
             study.optimize(func_value_error, n_trials=20, catch=(ArithmeticError, ))
+        assert len(study.trials) == 21
+        assert all(trial.state == optuna.structs.TrialState.FAIL for trial in study.trials)
 
 
 @pytest.mark.parametrize('storage_mode', STORAGE_MODES)


Code review comments:
diff_hunk: @@ -510,13 +510,16 @@ def _run_trial(self, func, catch):
             self.logger.info(message)
             self._storage.set_trial_state(trial_id, structs.TrialState.PRUNED)
             return trial
-        except catch as e:
+        except Exception as e:
             message = 'Setting status of trial#{} as {} because of the following error: {}'\
                 .format(trial_number, structs.TrialState.FAIL, repr(e))
             self.logger.warning(message, exc_info=True)
             self._storage.set_trial_system_attr(trial_id, 'fail_reason', message)
             self._storage.set_trial_state(trial_id, structs.TrialState.FAIL)
-            return trial
+
+            if any(isinstance(e, cls) for cls in catch):
path: optuna/study.py
text: @user1:
According to [the reference](https://docs.python.org/3/library/functions.html#isinstance), `isinstance` accepts a tuple of exceptions as a second argument. I think we can simplify this line as follows:
```suggestion
            if isinstance(e, catch):
```

But, I have a concern about this implementation. It does not accept a list of exceptions although some novice users may not care about the difference between `tuple` and `list`.
What do you think of it?

@user2:
The current type hint of `catch` argument is `Union[Tuple[()], Tuple[Type[Exception]]]`, so, IMO, we shouldn't accept `list`.
I think that it may be better to add a type check to prevent users from passing a list unintendedly.
(or change the type hint to accept list type).


@user1:
I agree with @user2.

> add a type check to prevent users from passing a list unintendedly.

That's a good idea. It would be helpful for novice users of Python to fix their code.

@author:
Added a type check in `optimize` (to keep the callstack shallow when raised).