slds-lmu / yahpo_gym

Surrogate benchmarks for HPO problems
26 stars 3 forks source link

LocalConfiguration nested duplicated __init__ without effect #70

Closed LGro closed 1 year ago

LGro commented 1 year ago

Looking at LocalConfiguration.__init__ I noticed that there is an __init__ definition inside the __init__ definition where my impression is that the inner one would never be called. Thus I'd expect the option to set YAHPO_LOCAL_CONFIG to never work. I'd be curious to hear about the ideas behind that construction and whether you'd also consider this a bug.

    def __init__(self, settings_path: str ="~/.config/yahpo_gym"):
        def __init__(self, settings_path: Optional[str] =None):
            if settings_path is None: 
                if 'YAHPO_LOCAL_CONFIG' in os.environ:
                    settings_path = os.environ['YAHPO_LOCAL_CONFIG']
                else: 
                    settings_path = "~/.config/yahpo_gym"
        self.settings_path = Path(settings_path).expanduser().absolute()
        self._config = None

Maybe a test like this could help check the intended behaviour:

import os

tmp_local_config_path = os.environ.get("YAHPO_LOCAL_CONFIG", None)
os.environ["YAHPO_LOCAL_CONFIG"] = "/tmp/yahpo_conf"
from yahpo_gym.configuration import local_config

assert str(local_config.settings_path) == os.environ["YAHPO_LOCAL_CONFIG"]

if tmp_local_config_path is not None:
    os.environ["YAHPO_LOCAL_CONFIG"] = tmp_local_config_path
pfistfl commented 1 year ago

Hey there! thanks a lot, this is indeed odd! I will address this!