Repository: benchopt/benchopt
Pull request #678 (2024-01-26 13:34:27)

FIX config set for yaml files
Also add some test.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/benchopt/benchopt/pull/678?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) Report
> Merging [#678](https://app.codecov.io/gh/benchopt/benchopt/pull/678?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) (87b3d57) into [main](https://app.codecov.io/gh/benchopt/benchopt/commit/7d0362c09ea015df70cd9541b2d966cad7f9e739?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=benchopt) (7d0362c) will **decrease** coverage by `0.29%`.
> Report is 1 commits behind head on main.
> The diff coverage is `5.26%`.

: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     #678      +/-   ##
==========================================
- Coverage   54.88%   54.60%   -0.29%     
==========================================
  Files          45       45              
  Lines        2928     2934       +6     
  Branches      539      542       +3     
==========================================
- Hits         1607     1602       -5     
- Misses       1196     1207      +11     
  Partials      125      125              
```



</details>

Patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
index ae0b4dd..2ee6ec6 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -58,7 +58,7 @@ jobs:
           command: |
             # remove the cache index
             mamba clean -i -y
-            mamba install python=3.9 julia r-base rpy2 \
+            mamba install python=3.10 julia r-base rpy2 \
                           numpy cython -yq
             pip install --upgrade "setuptools<58.5"
             pip install --upgrade --progress-bar off julia
diff --git a/benchopt/config.py b/benchopt/config.py
index 0803dd2..1330d5c 100644
--- a/benchopt/config.py
+++ b/benchopt/config.py
@@ -42,7 +42,8 @@ DEFAULT_GLOBAL_CONFIG = {
 """
 
 DEFAULT_BENCHMARK_CONFIG = {
-    "plots": list(PLOT_KINDS), "plot_configs": {}
+    "plots": list(PLOT_KINDS),
+    "plot_configs": {}
 }
 
 """
@@ -157,10 +158,15 @@ def set_setting(name, value, config_file=None, benchmark_name=None):
     if config_file is None:
         config_file = get_global_config_file()
 
+    # Handle deprecated .ini config file by automatically
+    # converting them to .yml.
+    if config_file.suffix == ".ini":
+        convert_ini_to_yml(config_file)
+        config_file = config_file.with_suffix('.yml')
+
     # Get default value
     default_config = DEFAULT_BENCHMARK_CONFIG
     if benchmark_name is None:
-        benchmark_name = 'benchopt'
         default_config = DEFAULT_GLOBAL_CONFIG
 
     if name not in default_config:
@@ -171,21 +177,25 @@ def set_setting(name, value, config_file=None, benchmark_name=None):
         raise SystemExit(1)
     default_value = default_config[name]
 
-    # Get global config file
-    config = configparser.ConfigParser()
-    config.read(config_file)
-
-    if benchmark_name not in config:
-        config[benchmark_name] = {}
+    # Load the config from the yaml file if the file exists.
+    config = {}
+    if config_file.exists():
+        with open(config_file, "r") as f:
+            config = yaml.safe_load(f) or {}
 
-    config[benchmark_name][name] = reverse_parse(default_value, value)
+    if benchmark_name is not None:
+        if benchmark_name not in config:
+            config[benchmark_name] = {}
+        config[benchmark_name][name] = reverse_parse(default_value, value)
+    else:
+        config[name] = reverse_parse(default_value, value)
 
     # Create config file with the correct permission.
     if not config_file.exists():
         config_file.touch(mode=GLOBAL_CONFIG_FILE_MODE)
 
     with config_file.open('w') as f:
-        config.write(f)
+        yaml.safe_dump(config, f)
 
 
 def get_setting(name, config_file=None, benchmark_name=None,
@@ -212,6 +222,12 @@ def get_setting(name, config_file=None, benchmark_name=None,
     if config_file is None:
         config_file = get_global_config_file()
 
+    # Handle deprecated .ini config file by automatically
+    # converting them to .yml.
+    if config_file.suffix == ".ini":
+        convert_ini_to_yml(config_file)
+        config_file = config_file.with_suffix('.yml')
+
     # Get default value
     default_config_ = DEFAULT_BENCHMARK_CONFIG
     if benchmark_name is None:
@@ -222,21 +238,14 @@ def get_setting(name, config_file=None, benchmark_name=None,
     default_config = {} if default_config is None else default_config
     default_value = default_config.get(name, default_config_[name])
 
-    # Handle deprecated .ini config file by automatically
-    # converting them to .yml.
-    if config_file.suffix == ".ini":
-        convert_ini_to_yml(config_file)
-        config_file = config_file.with_suffix('.yml')
-
     # Load the config from the yaml file if the file exists.
+    config = {}
     if config_file.exists():
         with open(config_file, "r") as f:
-            config = yaml.safe_load(f)
-    else:
-        config = {}
+            config = yaml.safe_load(f) or {}
 
     # Get value from config file or keep the default value.
-    if benchmark_name in config.keys():
+    if benchmark_name in config:
         value = config[benchmark_name].get(name, default_value)
     else:
         value = config.get(name, default_value)
diff --git a/setup.cfg b/setup.cfg
index e32ffaf..05a5a8a 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -61,7 +61,7 @@ test =
 
 doc =
     numpydoc
-    sphinx==4.5.0
+    sphinx
     sphinx-book-theme
     sphinx-design
     sphinx-click


Test patch:
diff --git a/benchopt/tests/test_config.py b/benchopt/tests/test_config.py
index 6372bdf..22f49d2 100644
--- a/benchopt/tests/test_config.py
+++ b/benchopt/tests/test_config.py
@@ -1,15 +1,19 @@
 import os
 import pytest
+import warnings
 from pathlib import Path
 from contextlib import contextmanager
 
+from benchopt.config import parse_value
+from benchopt.config import DEFAULT_GLOBAL_CONFIG
 from benchopt.config import get_global_config_file
+from benchopt.config import set_setting, get_setting
 
 
 @contextmanager
-def set_config_file(permission='600'):
+def temp_config_file(permission='600'):
     permission = int(f"100{permission}", base=8)
-    config_file = Path() / 'test_config_file.ini'
+    config_file = Path() / 'test_config_file.yml'
     config_file.touch(mode=permission, exist_ok=False)
     old_config_file = os.environ.get('BENCHOPT_CONFIG', None)
     os.environ['BENCHOPT_CONFIG'] = str(config_file)
@@ -23,9 +27,27 @@ def set_config_file(permission='600'):
         config_file.unlink()
 
 
+def test_parse_value():
+    # Check boolean parsing
+    for val in [True, 'true', 'True', '1', 'yes', 'on']:
+        assert parse_value(val, True) is True
+    for val in ['false', 'False', '0', 'no', 'off']:
+        assert parse_value(val, True) is False
+
+    # Check list parsing
+    value = "test, test1\ntest2     \n test3"
+    assert parse_value(value, []) == ['test', 'test1', 'test2', 'test3']
+
+    # Check that if the default is not a bool or a list, we return the value
+    assert parse_value(1, 2) == 1
+    assert parse_value(2.0, 1) == 2.0
+    assert parse_value('abc', 1) == 'abc'
+    assert parse_value('abc', 'a') == 'abc'
+
+
 @pytest.mark.parametrize("permission", ["644", "655", "240"])
 def test_config_file_permission_warn(permission):
-    with set_config_file(permission) as config_file:
+    with temp_config_file(permission) as config_file:
         msg = f"{config_file} is with mode {permission}"
         with pytest.warns(UserWarning, match=msg):
             global_config_file = get_global_config_file()
@@ -33,11 +55,42 @@ def test_config_file_permission_warn(permission):
 
 
 def test_config_file_permission_no_warning():
-    with set_config_file() as config_file:
-        with pytest.warns(None) as record:
+    with temp_config_file() as config_file:
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
             global_config_file = get_global_config_file()
         assert str(global_config_file) == str(config_file)
-        assert len(record) == 0, (
-            "A warning was issued while it should not have.\n"
-            f"Warning is: {record[0]}"
-        )
+
+
+@pytest.mark.parametrize("setting_key", DEFAULT_GLOBAL_CONFIG)
+def test_config_file_set(setting_key):
+    with temp_config_file():
+        default_value = DEFAULT_GLOBAL_CONFIG[setting_key]
+        default_value = parse_value(os.environ.get(
+            f"BENCHOPT_{setting_key.upper()}", default_value
+        ), default_value)
+        assert get_setting(setting_key) == default_value
+
+        set_value = parse_value('True', default_value)
+        set_setting(setting_key, set_value)
+
+        # Make sure the value is correctly set and retrieved
+        try:
+            # If the env var is not set, we should get the set value
+            old_value = os.environ.pop(f"BENCHOPT_{setting_key.upper()}", None)
+            assert get_setting(setting_key) == set_value
+
+            # Otherwise, we get the env value
+            env_value = parse_value('False', default_value)
+            os.environ[f"BENCHOPT_{setting_key.upper()}"] = str(env_value)
+            assert get_setting(setting_key) == env_value
+            del os.environ[f"BENCHOPT_{setting_key.upper()}"]
+        finally:
+            if old_value is not None:
+                os.environ[f"BENCHOPT_{setting_key.upper()}"] = old_value
+
+
+def test_config_file_set_error():
+    with temp_config_file():
+        with pytest.raises(SystemExit):
+            set_setting('invalid_key', None)