Repository: benchopt/benchopt
Pull request #763 (2024-08-02 12:03:48)

Fix skip objective
The current implementation of the `skip` API fails when skipping some objective, because of bad interactions with the terminal_output.
This PR fixes the issue and adds better tests for the full pipeline, including reporting.

cc @ceelestin 

- [x] Add tests
- [x] Add what's new entry

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/benchopt/benchopt/pull/763?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 82.44%. Comparing base [(`011946a`)](https://app.codecov.io/gh/benchopt/benchopt/commit/011946acd685783777426420117b63155ba18fe0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) to head [(`7e5b836`)](https://app.codecov.io/gh/benchopt/benchopt/commit/7e5b836bd78ad5805c7508d704cb96b485dffddc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   82.15%   82.44%   +0.28%     
==========================================
  Files          46       46              
  Lines        3139     3139              
==========================================
+ Hits         2579     2588       +9     
+ Misses        560      551       -9     
```

</details>

Patch:
diff --git a/benchopt/utils/terminal_output.py b/benchopt/utils/terminal_output.py
index c8a7040..def94b3 100644
--- a/benchopt/utils/terminal_output.py
+++ b/benchopt/utils/terminal_output.py
@@ -84,7 +84,7 @@ class TerminalOutput:
         new_output = TerminalOutput(self.n_repetitions, self.show_progress)
         new_output.set(
             solver=self.solver, dataset=self.dataset, objective=self.objective,
-            verbose=self.verbose, rep=self.rep
+            verbose=self.verbose, rep=self.rep, i_solver=self.i_solver
         )
         return new_output
 
@@ -147,7 +147,7 @@ class TerminalOutput:
 
     def show_status(self, status, dataset=False, objective=False):
         if dataset or objective:
-            assert status == 'not installed'
+            assert status in ['not installed', 'skip']
         tag = (
             self.dataset_tag if dataset else
             self.objective_tag if objective else self.solver_tag
diff --git a/doc/whats_new.rst b/doc/whats_new.rst
index d9e56a1..bf93e25 100644
--- a/doc/whats_new.rst
+++ b/doc/whats_new.rst
@@ -29,6 +29,9 @@ FIX
 - Default value for ``data_home`` was incorrect.
   By `Thomas Moreau`_ (:gh:`758`)
 
+- Fix the ``skip`` API for objectives that was leading to a display error.
+  By `Thomas Moreau`_ (:gh:`763`)
+
 
 .. _changes_1_6:
 


Test patch:
diff --git a/benchopt/tests/__init__.py b/benchopt/tests/__init__.py
index 26b4742..a68d45f 100644
--- a/benchopt/tests/__init__.py
+++ b/benchopt/tests/__init__.py
@@ -24,11 +24,8 @@ except Exception:
     DUMMY_BENCHMARK = None
 try:
     TEST_OBJECTIVE = DUMMY_BENCHMARK.get_benchmark_objective()
-    TEST_SOLVER = [s for s in DUMMY_BENCHMARK.get_solvers()
-                   if s.name == "Solver-Test"][0]
     TEST_DATASET = [d for d in DUMMY_BENCHMARK.get_datasets()
                     if d.name == "Test-Dataset"][0]
 except Exception:
     TEST_OBJECTIVE = None
-    TEST_SOLVER = None
     TEST_DATASET = None
diff --git a/benchopt/tests/dummy_benchmark/solvers/solver_test.py b/benchopt/tests/dummy_benchmark/solvers/solver_test.py
index bcfdc75..e559eb9 100644
--- a/benchopt/tests/dummy_benchmark/solvers/solver_test.py
+++ b/benchopt/tests/dummy_benchmark/solvers/solver_test.py
@@ -18,11 +18,6 @@ class Solver(BaseSolver):
     def __init__(self, raise_error=False):
         self.raise_error = raise_error
 
-    def skip(self, X, y, lmbd):
-        if lmbd == 0:
-            return True, "lmbd=0"
-        return False, None
-
     def set_objective(self, X, y, lmbd):
         self.X, self.y, self.lmbd = X, y, lmbd
 
diff --git a/benchopt/tests/test_runner.py b/benchopt/tests/test_runner.py
index adfff78..ffb9cf2 100644
--- a/benchopt/tests/test_runner.py
+++ b/benchopt/tests/test_runner.py
@@ -3,7 +3,6 @@ import numpy as np
 
 import pytest
 
-from benchopt.tests import TEST_SOLVER
 from benchopt.tests import TEST_DATASET
 from benchopt.tests import TEST_OBJECTIVE
 
@@ -20,40 +19,63 @@ from benchopt.tests.utils import CaptureRunOutput
 from benchopt.cli.main import run
 
 
-class MockOutput:
-    def __init__(self):
-        self.reason = None
+@pytest.mark.parametrize('n_jobs', [1, 2, 4])
+def test_skip_api(n_jobs):
 
-    def skip(self, reason):
-        self.reason = reason
+    objective = """from benchopt import BaseObjective
 
+        class Objective(BaseObjective):
+            name = "Objective-skip"
+            parameters = dict(should_skip=[True, False])
 
-def test_skip_api():
+            def skip(self, X, y):
+                if self.should_skip:
+                    return True, "Objective$skip"
+                return False, None
 
-    dataset = TEST_DATASET.get_instance()
-    objective = TEST_OBJECTIVE.get_instance(reg=0)
-    objective.set_dataset(dataset)
+            def set_data(self, X, y): self.X, self.y = X, y
+            def get_objective(self): return dict(X=1)
+            def get_one_result(self): return dict(beta=0)
+            def evaluate_result(self, beta): return dict(value=1)
+    """
 
-    solver = TEST_SOLVER.get_instance()
+    solver = """from benchopt import BaseSolver
 
-    out = MockOutput()
-    skip = solver._set_objective(objective, out)
-    assert skip
-    assert out.reason == 'lmbd=0'
+    class Solver(BaseSolver):
+        name = "test-solver"
+        sampling_strategy = 'run_once'
+        parameters = dict(should_skip=[True, False])
 
-    objective = TEST_OBJECTIVE.get_instance(reg=1)
-    objective.set_dataset(dataset)
+        def skip(self, X):
+            if self.should_skip:
+                return True, "Solver$skip"
+            return False, None
 
-    out = MockOutput()
-    skip = solver._set_objective(objective, out)
-    assert not skip
-    assert out.reason is None
+        def set_objective(self, X): pass
+        def run(self, n_iter): print("RUN")
+        def get_result(self): return dict(beta=1)
+    """
 
-    dataset = TEST_DATASET.get_instance(skip=True)
-    objective = TEST_OBJECTIVE.get_instance()
-    skip, reason = objective.set_dataset(dataset)
-    assert skip
-    assert reason == 'X is all zeros'
+    with temp_benchmark(objective=objective, solvers=[solver]) as benchmark:
+        with CaptureRunOutput() as out:
+            run([*(
+                f'{benchmark.benchmark_dir} -s test-solver -d test-dataset '
+                f'-j {n_jobs} --no-plot'
+            ).split()], standalone_mode=False)
+
+            # Make sure joblib's executor is shutdown, as otherwise the output
+            # might be incomplete.
+            from joblib.externals.loky import get_reusable_executor
+            get_reusable_executor().shutdown(wait=True)
+
+    out.check_output(r"Objective-skip\[should_skip=True\] skip", repetition=1)
+    out.check_output(r"Reason: Objective\$skip", repetition=1)
+
+    out.check_output(r"test-solver\[should_skip=True\]: skip", repetition=1)
+    out.check_output(r"Reason: Solver\$skip", repetition=1)
+
+    out.check_output(r"test-solver\[should_skip=False\]: done", repetition=1)
+    out.check_output("RUN", repetition=1)
 
 
 def test_get_one_result():
diff --git a/benchopt/tests/utils/capture_run_output.py b/benchopt/tests/utils/capture_run_output.py
index e66ffef..25cc017 100644
--- a/benchopt/tests/utils/capture_run_output.py
+++ b/benchopt/tests/utils/capture_run_output.py
@@ -54,7 +54,14 @@ class CaptureRunOutput(object):
 
     def check_output(self, pattern, repetition=None):
         output = self.output
+
+        # Remove color for matches
+        for c in range(30, 38):
+            output = output.replace(f"\033[1;{c}m", "")
+        output = output.replace("\033[0m", "")
+
         matches = re.findall(pattern, output)
+
         if repetition is None:
             assert len(matches) > 0, (
                 f"Could not find '{pattern}' in output:\n{output}"