Repository: optuna/optuna
Pull request: `optuna tell` with `--skip-if-finished`
<!-- Thank you for creating a pull request! In general, we merge your pull requests after they get two or more approvals. -->

## Motivation

See description.

## Description of the changes

Fixes https://github.com/optuna/optuna/issues/3130.

Problem statement:
Preemptible instances support of `optuna tell`.
## Motivation

When using optuna ask&tell on preemptible instances, `optuna tell` can raise `RuntimeError` on preemption because the trial can be already in one of `finished state` after preemption.

```shell
... # Some codes
optuna tell --storage $OPTUNA_STORAGE --study-name mystudy --trial-number 0 --values 1.0  # error in the second run
# <- preemption in the first run
... # Other codes
```

## Description

Return a different exit code when the trial is already in a `finished state` so that users can distinguish errors caused by preemption and others (e.g., connection error to DB).

## Alternatives

* Check the trial's state and values first and avoid calling `study.tell()` when the requested (state, values) are identical to the current value of the trial.
  * Concern: Comparison of values can be problematic due to precision issues of numerical values.
* Guide users to write python script and use `study.tell` + `try-catch` on preemptible instances. (I'm currently using this.)

## Additional context

I'm not quite sure a common way to handle this kind of situation and love to hear others' voice.

Hints from issue discussion:
Do you think it's common to then branch the post-tell logic depending on the error code here? We could also as an additional alternative add an option like `--skip-if-told`, which would be similar to [`optuna create-study`'s `--skip-if-exists`](https://github.com/optuna/optuna/blob/release-v2.10.0/optuna/cli.py#L249-L255).
> We could also as an additional alternative add an option like `--skip-if-told`, which would be similar to `optuna create-study`'s `--skip-if-exists`.

Thanks for the reply. This option sounds more consistent and clear.
It should be easy to implement, let me work on that.

Commit message: optuna tell with --skip-if-finished

Patch to review:
diff --git a/optuna/cli.py b/optuna/cli.py
index 2c57d88ea41..a381616cf87 100644
--- a/optuna/cli.py
+++ b/optuna/cli.py
@@ -838,6 +838,13 @@ def get_parser(self, prog_name: str) -> ArgumentParser:
         parser.add_argument("--trial-number", type=int, help="Trial number.")
         parser.add_argument("--values", type=float, nargs="+", help="Objective values.")
         parser.add_argument("--state", type=str, default="complete", help="Trial state.")
+        parser.add_argument(
+            "--skip-if-finished",
+            default=False,
+            action="store_true",
+            help="If specified, tell is skipped without any error when the trial is already"
+            "finished.",
+        )
         return parser
 
     def take_action(self, parsed_args: Namespace) -> int:
@@ -855,16 +862,26 @@ def take_action(self, parsed_args: Namespace) -> int:
         )
 
         state = TrialState[parsed_args.state.upper()]
-        study.tell(
-            trial=parsed_args.trial_number,
-            values=parsed_args.values,
-            state=state,
-        )
+        trial_number = parsed_args.trial_number
+        values = parsed_args.values
+
+        # TODO(hvy): Consider introducing `skip_if_finished` to `Study.tell` and let the method
+        # do all error handling, such as invalid trial numbers.
+        if parsed_args.skip_if_finished:
+            trials = study.get_trials(deepcopy=False)
+            trial = trials[trial_number]
+            assert trial.number == trial_number
+
+            if trial.state.is_finished():
+                self.logger.info(
+                    f"Skipped telling trial {trial_number} with values "
+                    f"{values} and state {state} since trial was already finished."
+                )
+                return 0
 
-        self.logger.info(
-            f"Told trial {parsed_args.trial_number} with values {parsed_args.values} and state "
-            f"{state}."
-        )
+        study.tell(trial=trial_number, values=values, state=state)
+
+        self.logger.info(f"Told trial {trial_number} with values {values} and state " f"{state}.")
 
         return 0
 
diff --git a/tests/test_cli.py b/tests/test_cli.py
index b15f20cf0dc..226cd07de4b 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -1444,7 +1444,7 @@ def test_tell() -> None:
         output = json.loads(output)
         trial_number = output["number"]
 
-        output = subprocess.check_output(
+        subprocess.check_output(
             [
                 "optuna",
                 "tell",
@@ -1461,3 +1461,38 @@ def test_tell() -> None:
         assert len(study.trials) == 1
         assert study.trials[0].state == TrialState.COMPLETE
         assert study.trials[0].values == [1.2]
+
+        # Error when updating a finished trial.
+        ret = subprocess.run(
+            [
+                "optuna",
+                "tell",
+                "--storage",
+                db_url,
+                "--trial-number",
+                str(trial_number),
+                "--values",
+                "1.2",
+            ]
+        )
+        assert ret.returncode != 0
+
+        # Passing `--skip-if-finished` to a finished trial for a noop.
+        subprocess.check_output(
+            [
+                "optuna",
+                "tell",
+                "--storage",
+                db_url,
+                "--trial-number",
+                str(trial_number),
+                "--values",
+                "1.3",  # Setting a different value and make sure it's not persisted.
+                "--skip-if-finished",
+            ]
+        )
+
+        study = optuna.load_study(storage=db_url, study_name=study_name)
+        assert len(study.trials) == 1
+        assert study.trials[0].state == TrialState.COMPLETE
+        assert study.trials[0].values == [1.2]


Code review comments:
diff_hunk: @@ -855,16 +862,26 @@ def take_action(self, parsed_args: Namespace) -> int:
         )
 
         state = TrialState[parsed_args.state.upper()]
-        study.tell(
-            trial=parsed_args.trial_number,
-            values=parsed_args.values,
-            state=state,
-        )
+        trial_number = parsed_args.trial_number
+        values = parsed_args.values
+
+        # TODO(hvy): Consider introducing `skip_if_finished` to `Study.tell` and let the method
+        # do all error handling, such as invalid trial numbers.
+        if parsed_args.skip_if_finished:
+            trials = study.get_trials(deepcopy=False)
+            trial = trials[trial_number]
+            assert trial.number == trial_number
+
+            if trial.state.is_finished():
+                self.logger.info(
+                    f"Skipped telling trial {trial_number} with values "
+                    f"{values} and state {state} since trial was already finished."
+                )
path: optuna/cli.py
text: @user1:
How about logging `trial.state` and `trial.values` as well as `state` and `values`?

@author:
Sounds good. I'll fix that.

@author:
Just fixed, PTAL if you have time over 🙇 

@user1:
Thank you for the swift fix!