Repository: benchopt/benchopt
Pull request #686 (2024-03-23 22:12:09)

FIX sampling strategy priority
Fix the regression introduced in #623 with `run_once` and add some non-regression tests.
This should fix the failures seen on [benchopt/benchmark_bilevel](https://github.com/benchopt/benchmark_bilevel)



codecov-commenter: ## [Codecov](https://app.codecov.io/gh/benchopt/benchopt/pull/686?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) Report
> Merging [#686](https://app.codecov.io/gh/benchopt/benchopt/pull/686?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) (ea3f3e5) into [main](https://app.codecov.io/gh/benchopt/benchopt/commit/fd0a0169c6738cb497794a09be415dbd144d3c49?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) (fd0a016) will **increase** coverage by `0.04%`.
> The diff coverage is `81.25%`.

> :exclamation: Current head ea3f3e5 differs from pull request most recent head 1619cda. Consider uploading reports for the commit 1619cda to get more accurate results

:exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

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


```diff
@@            Coverage Diff             @@
##             main     #686      +/-   ##
==========================================
+ Coverage   55.03%   55.07%   +0.04%     
==========================================
  Files          45       45              
  Lines        2962     2967       +5     
  Branches      548      551       +3     
==========================================
+ Hits         1630     1634       +4     
  Misses       1209     1209              
- Partials      123      124       +1     
```



</details>

Patch:
diff --git a/benchopt/base.py b/benchopt/base.py
index 24b3ef6..9a4f188 100644
--- a/benchopt/base.py
+++ b/benchopt/base.py
@@ -48,10 +48,12 @@ class BaseSolver(ParametrizedNameMixin, DependenciesMixin, ABC):
     """
 
     _base_class_name = 'Solver'
-    sampling_strategy = 'iteration'
+    sampling_strategy = None
 
     @property
-    def stopping_criterion(self):
+    def _stopping_criterion(self):
+        if hasattr(self, 'stopping_criterion'):
+            return self.stopping_criterion
         if self.sampling_strategy == 'run_once':
             return SingleRunCriterion()
         return SufficientProgressCriterion(strategy=self.sampling_strategy)
@@ -59,10 +61,10 @@ class BaseSolver(ParametrizedNameMixin, DependenciesMixin, ABC):
     @property
     def _solver_strategy(self):
         """Change stop_strategy and stopping_strategy to sampling_strategy."""
-        if hasattr(self, 'sampling_strategy'):
-            return self.sampling_strategy
-        else:
-            return self.stopping_criterion.strategy
+        return (
+            self._stopping_criterion.strategy or self.sampling_strategy
+            or 'iteration'
+        )
 
     def _set_objective(self, objective, output=None):
         """Store the objective for hashing/pickling and check its compatibility
diff --git a/benchopt/runner.py b/benchopt/runner.py
index 4bf12fd..28c9763 100644
--- a/benchopt/runner.py
+++ b/benchopt/runner.py
@@ -220,7 +220,7 @@ def run_one_solver(benchmark, dataset, objective, solver, n_repetitions,
             solver_description=inspect.cleandoc(solver.__doc__ or ""),
         )
 
-        stopping_criterion = solver.stopping_criterion.get_runner_instance(
+        stopping_criterion = solver._stopping_criterion.get_runner_instance(
             solver=solver,
             max_runs=max_runs,
             timeout=timeout / n_repetitions,
diff --git a/benchopt/stopping_criterion.py b/benchopt/stopping_criterion.py
index 995053f..fa66df8 100644
--- a/benchopt/stopping_criterion.py
+++ b/benchopt/stopping_criterion.py
@@ -64,9 +64,11 @@ class StoppingCriterion():
     def __init__(self, strategy=None, key_to_monitor='objective_value',
                  **kwargs):
 
-        assert strategy in SAMPLING_STRATEGIES, (
-            f"strategy should be in {SAMPLING_STRATEGIES}. Got '{strategy}'."
-        )
+        if strategy is not None:
+            assert strategy in SAMPLING_STRATEGIES, (
+                f"strategy should be in {SAMPLING_STRATEGIES}. "
+                f"Got '{strategy}'."
+            )
 
         self.kwargs = kwargs
         self.strategy = strategy
@@ -107,6 +109,14 @@ class StoppingCriterion():
                 "to implement a new StoppingCriterion."
             )
 
+        if self.strategy is None:
+            self.strategy = solver.sampling_strategy or 'iteration'
+        elif solver is not None and solver.sampling_strategy is not None:
+            assert solver.sampling_strategy == self.strategy, (
+                'The strategy is set both in Solver.sampling_strategy and in '
+                'its criterion, and it does not match. Only set it once.'
+            )
+
         # Create a new instance of the class
         stopping_criterion = self.__class__(
             strategy=self.strategy, key_to_monitor=self.key_to_monitor,
@@ -305,7 +315,7 @@ class SufficientDescentCriterion(StoppingCriterion):
         updates.{COMMON_ARGS_DOC}
     """
 
-    def __init__(self, eps=EPS, patience=PATIENCE, strategy='iteration',
+    def __init__(self, eps=EPS, patience=PATIENCE, strategy=None,
                  key_to_monitor='objective_value'):
         self.eps = eps
         self.patience = patience
@@ -373,7 +383,7 @@ class SufficientProgressCriterion(StoppingCriterion):
         updates.{COMMON_ARGS_DOC}
     """
 
-    def __init__(self, eps=EPS, patience=PATIENCE, strategy='iteration',
+    def __init__(self, eps=EPS, patience=PATIENCE, strategy=None,
                  key_to_monitor='objective_value'):
         self.eps = eps
         self.patience = patience
@@ -441,10 +451,10 @@ class SingleRunCriterion(StoppingCriterion):
         minus one for the ``'callback'`` strategy.
     """
 
-    def __init__(self, stop_val=1, *args, **kwargs):
+    def __init__(self, stop_val=1, strategy=None, *args, **kwargs):
         # Necessary as the criterion is given a strategy argument when
         # instanciated for an instance.
-        super().__init__(strategy="iteration", stop_val=stop_val)
+        super().__init__(strategy=strategy, stop_val=stop_val)
         self.stop_val = stop_val
 
     def init_stop_val(self):
diff --git a/setup.cfg b/setup.cfg
index 2fc7fed..b2d5411 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -74,3 +74,6 @@ doc =
 slurm =
     submitit
     rich
+
+[flake8]
+exclude = benchmarks,__cache__


Test patch:
diff --git a/benchopt/tests/fixtures.py b/benchopt/tests/fixtures.py
index c77b4d9..56c48f4 100644
--- a/benchopt/tests/fixtures.py
+++ b/benchopt/tests/fixtures.py
@@ -29,6 +29,8 @@ def pytest_report_header(config):
 def pytest_addoption(parser):
     parser.addoption("--skip-install", action="store_true",
                      help="skip install of solvers that can slow down CI.")
+    parser.addoption("--skip-env", action="store_true",
+                     help="skip tests which requires creating a conda env.")
     parser.addoption("--test-env", type=str, default=None,
                      help="Use a given env to test the solvers' install.")
     parser.addoption("--recreate", action="store_true",
@@ -122,6 +124,8 @@ def test_env_name(request):
     global _TEST_ENV_NAME
 
     if _TEST_ENV_NAME is None:
+        if request.config.getoption("--skip-env"):
+            pytest.skip("Skip creating a test env")
         env_name = request.config.getoption("--test-env")
         recreate = request.config.getoption("--recreate")
         if env_name is None:
diff --git a/benchopt/tests/test_stopping_criterion.py b/benchopt/tests/test_stopping_criterion.py
index fbeda3f..b0c7749 100644
--- a/benchopt/tests/test_stopping_criterion.py
+++ b/benchopt/tests/test_stopping_criterion.py
@@ -1,10 +1,27 @@
 import pytest
 import numpy as np
 
+from benchopt.cli.main import run
+from benchopt.tests.utils import CaptureRunOutput
+from benchopt.utils.temp_benchmark import temp_benchmark
+
 from benchopt.stopping_criterion import SAMPLING_STRATEGIES
+from benchopt.stopping_criterion import SingleRunCriterion
 from benchopt.stopping_criterion import SufficientDescentCriterion
 from benchopt.stopping_criterion import SufficientProgressCriterion
 
+MINIMAL_OBJECTIVE = """from benchopt import BaseObjective
+
+    class Objective(BaseObjective):
+        name = "stopping_criterion"
+        min_benchopt_version = "0.0.0"
+
+        def set_data(self, X, y): self.X, self.y = X, y
+        def get_objective(self): return {}
+        def get_one_result(self): return dict(beta=0)
+        def evaluate_result(self, beta): return dict(value=1)
+"""
+
 
 @pytest.mark.parametrize('strategy', SAMPLING_STRATEGIES)
 @pytest.mark.parametrize('criterion_class', [
@@ -92,3 +109,145 @@ def test_key_to_monitor(criterion_class, strategy):
     stop, status, stop_val = criterion.should_stop(stop_val, objective_list)
     assert stop, "Should have stopped"
     assert status == 'diverged', "Should stop on diverged"
+
+
+@pytest.mark.parametrize('strategy', SAMPLING_STRATEGIES)
+def test_solver_strategy(no_debug_test, strategy):
+
+    solver = f"""from benchopt import BaseSolver
+
+    class Solver(BaseSolver):
+        name = "test-solver"
+        sampling_strategy = "{strategy}"
+        def set_objective(self): pass
+        def run(self, n_iter):
+            assert self._solver_strategy == "{strategy}", self._solver_strategy
+            if self._solver_strategy in "iteration":
+                assert n_iter == 0, n_iter
+            elif self._solver_strategy in "tolerance":
+                assert n_iter == 3e38, n_iter
+            elif self._solver_strategy in "run_once":
+                assert n_iter == 1, n_iter
+            elif self._solver_strategy in "callback":
+                assert callable(n_iter)
+                while n_iter(): pass
+
+        def get_result(self): return dict(beta=1)
+    """
+
+    with temp_benchmark(
+            objective=MINIMAL_OBJECTIVE,
+            solvers=[solver]
+    ) as benchmark:
+        with CaptureRunOutput():
+            run([str(benchmark.benchmark_dir),
+                *('-s test-solver -d test-dataset --no-plot -n 0').split()],
+                standalone_mode=False)
+
+
+@pytest.mark.parametrize('strategy', SAMPLING_STRATEGIES)
+@pytest.mark.parametrize('criterion_class', [
+    SufficientDescentCriterion, SufficientProgressCriterion
+])
+def test_stopping_criterion_strategy(no_debug_test, criterion_class, strategy):
+
+    if strategy == "run_once":
+        criterion_class = SingleRunCriterion
+
+    solver = f"""from benchopt import BaseSolver
+    from benchopt.stopping_criterion import *
+
+    class Solver(BaseSolver):
+        name = "test-solver"
+        stopping_criterion = {criterion_class.__name__}(strategy="{strategy}")
+        def set_objective(self): pass
+        def run(self, n_iter):
+            assert self._solver_strategy == "{strategy}", self._solver_strategy
+            if self._solver_strategy in "iteration":
+                assert n_iter == 0, n_iter
+            elif self._solver_strategy in "tolerance":
+                assert n_iter == 3e38, n_iter
+            elif self._solver_strategy in "run_once":
+                assert n_iter == 1, n_iter
+            elif self._solver_strategy in "callback":
+                assert callable(n_iter)
+                while n_iter(): pass
+
+        def get_result(self): return dict(beta=1)
+    """
+
+    with temp_benchmark(
+            objective=MINIMAL_OBJECTIVE,
+            solvers=[solver]
+    ) as benchmark:
+        with CaptureRunOutput():
+            run([str(benchmark.benchmark_dir),
+                *('-s test-solver -d test-dataset --no-plot -n 0').split()],
+                standalone_mode=False)
+
+
+@pytest.mark.parametrize('strategy', SAMPLING_STRATEGIES)
+@pytest.mark.parametrize('criterion_class', [
+    SufficientDescentCriterion, SufficientProgressCriterion
+])
+def test_solver_override_strategy(no_debug_test, criterion_class, strategy):
+
+    if strategy == "run_once":
+        criterion_class = SingleRunCriterion
+
+    solver = f"""from benchopt import BaseSolver
+    from benchopt.stopping_criterion import *
+
+    class Solver(BaseSolver):
+        name = "test-solver"
+        sampling_strategy = "{strategy}"
+        stopping_criterion = {criterion_class.__name__}()
+        def set_objective(self): pass
+        def run(self, n_iter):
+            assert self._solver_strategy == "{strategy}", self._solver_strategy
+            if self._solver_strategy in "iteration":
+                assert n_iter == 0, n_iter
+            elif self._solver_strategy in "tolerance":
+                assert n_iter == 3e38, n_iter
+            elif self._solver_strategy in "run_once":
+                assert n_iter == 1, n_iter
+            elif self._solver_strategy in "callback":
+                assert callable(n_iter)
+                while n_iter(): pass
+
+        def get_result(self): return dict(beta=1)
+    """
+
+    with temp_benchmark(
+            objective=MINIMAL_OBJECTIVE,
+            solvers=[solver]
+    ) as benchmark:
+        with CaptureRunOutput():
+            run([str(benchmark.benchmark_dir),
+                *('-s test-solver -d test-dataset --no-plot -n 0').split()],
+                standalone_mode=False)
+
+
+def test_dual_strategy(no_debug_test):
+
+    solver = """from benchopt import BaseSolver
+    from benchopt.stopping_criterion import SufficientDescentCriterion
+
+    class Solver(BaseSolver):
+        name = "test-solver"
+        sampling_strategy = "iteration"
+        stopping_criterion = SufficientDescentCriterion(strategy='tolerance')
+        def set_objective(self): pass
+        def run(self, n_iter): pass
+        def get_result(self): return dict(beta=1)
+    """
+
+    with temp_benchmark(
+            objective=MINIMAL_OBJECTIVE,
+            solvers=[solver]
+    ) as benchmark:
+        with pytest.raises(AssertionError, match="Only set it once."):
+            with CaptureRunOutput():
+                run([str(benchmark.benchmark_dir),
+                    *('-s test-solver -d test-dataset --no-plot').split()],
+                    standalone_mode=False)