From c6ba0e8339004d53beace629ff53f2e220d1ad90 Mon Sep 17 00:00:00 2001
From: Graham Neubig <neubig@gmail.com>
Date: Wed, 28 Aug 2024 15:05:49 -0400
Subject: [PATCH] Remove singleton config (#3614)

* Remove singleton config

* Fix tests

* Fix logging reset

* Fix pre-commit
---
 openhands/core/config.py          | 14 +++++++-----
 openhands/core/utils/__init__.py  |  3 ---
 openhands/core/utils/singleton.py | 37 -------------------------------
 tests/unit/test_config.py         |  9 ++++----
 tests/unit/test_logging.py        |  3 ---
 5 files changed, 13 insertions(+), 53 deletions(-)
 delete mode 100644 openhands/core/utils/singleton.py

diff --git a/openhands/core/config.py b/openhands/core/config.py
index eefa35b0b3..3537522ad9 100644
--- a/openhands/core/config.py
+++ b/openhands/core/config.py
@@ -12,7 +12,6 @@ import toml
 from dotenv import load_dotenv
 
 from openhands.core import logger
-from openhands.core.utils import Singleton
 
 load_dotenv()
 
@@ -143,7 +142,7 @@ class AgentConfig:
 
 
 @dataclass
-class SecurityConfig(metaclass=Singleton):
+class SecurityConfig:
     """Configuration for security related functionalities.
 
     Attributes:
@@ -176,7 +175,7 @@ class SecurityConfig(metaclass=Singleton):
 
 
 @dataclass
-class SandboxConfig(metaclass=Singleton):
+class SandboxConfig:
     """Configuration for the sandbox.
 
     Attributes:
@@ -243,7 +242,7 @@ class UndefinedString(str, Enum):
 
 
 @dataclass
-class AppConfig(metaclass=Singleton):
+class AppConfig:
     """Configuration for the app.
 
     Attributes:
@@ -567,7 +566,12 @@ def load_from_toml(cfg: AppConfig, toml_file: str = 'config.toml'):
             sandbox_config = SandboxConfig(**toml_config['sandbox'])
 
         # update the config object with the new values
-        AppConfig(sandbox=sandbox_config, **core_config)
+        cfg.sandbox = sandbox_config
+        for key, value in core_config.items():
+            if hasattr(cfg, key):
+                setattr(cfg, key, value)
+            else:
+                logger.openhands_logger.warning(f'Unknown core config key: {key}')
     except (TypeError, KeyError) as e:
         logger.openhands_logger.warning(
             f'Cannot parse config from toml, toml values have not been applied.\nError: {e}',
diff --git a/openhands/core/utils/__init__.py b/openhands/core/utils/__init__.py
index 7cf6db84fd..e69de29bb2 100644
--- a/openhands/core/utils/__init__.py
+++ b/openhands/core/utils/__init__.py
@@ -1,3 +0,0 @@
-from openhands.core.utils.singleton import Singleton
-
-__all__ = ['Singleton']
diff --git a/openhands/core/utils/singleton.py b/openhands/core/utils/singleton.py
deleted file mode 100644
index dcaaf6583c..0000000000
--- a/openhands/core/utils/singleton.py
+++ /dev/null
@@ -1,37 +0,0 @@
-import dataclasses
-
-from openhands.core import logger
-
-
-class Singleton(type):
-    _instances: dict = {}
-
-    def __call__(cls, *args, **kwargs):
-        if cls not in cls._instances:
-            cls._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs)
-        else:
-            # allow updates, just update existing instance
-            # perhaps not the most orthodox way to do it, though it simplifies client code
-            # useful for pre-defined groups of settings
-            instance = cls._instances[cls]
-            for key, value in kwargs.items():
-                if hasattr(instance, key):
-                    setattr(instance, key, value)
-                else:
-                    logger.openhands_logger.warning(
-                        f'Unknown key for {cls.__name__}: "{key}"'
-                    )
-        return cls._instances[cls]
-
-    @classmethod
-    def reset(cls):
-        # used by pytest to reset the state of the singleton instances
-        for instance_type, instance in cls._instances.items():
-            print('resetting... ', instance_type)
-            for field_info in dataclasses.fields(instance_type):
-                if dataclasses.is_dataclass(field_info.type):
-                    setattr(instance, field_info.name, field_info.type())
-                elif field_info.default_factory is not dataclasses.MISSING:
-                    setattr(instance, field_info.name, field_info.default_factory())
-                else:
-                    setattr(instance, field_info.name, field_info.default)
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index 711edd5d10..4ee0552213 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -40,7 +40,6 @@ def temp_toml_file(tmp_path):
 @pytest.fixture
 def default_config(monkeypatch):
     # Fixture to provide a default AppConfig instance
-    AppConfig.reset()
     yield AppConfig()
 
 
@@ -501,8 +500,8 @@ def test_api_keys_repr_str():
 def test_max_iterations_and_max_budget_per_task_from_toml(temp_toml_file):
     temp_toml = """
 [core]
-max_iterations = 100
-max_budget_per_task = 4.0
+max_iterations = 42
+max_budget_per_task = 4.7
 """
 
     config = AppConfig()
@@ -511,8 +510,8 @@ max_budget_per_task = 4.0
 
     load_from_toml(config, temp_toml_file)
 
-    assert config.max_iterations == 100
-    assert config.max_budget_per_task == 4.0
+    assert config.max_iterations == 42
+    assert config.max_budget_per_task == 4.7
 
 
 def test_get_llm_config_arg(temp_toml_file):
diff --git a/tests/unit/test_logging.py b/tests/unit/test_logging.py
index 20f911da8e..5f5ef0b579 100644
--- a/tests/unit/test_logging.py
+++ b/tests/unit/test_logging.py
@@ -87,9 +87,6 @@ def test_app_config_attributes_masking(test_handler):
     assert 'e2b-xyz789' not in log_output
     assert 'ghp_abcdefghijklmnopqrstuvwxyz' not in log_output
 
-    # reset the AppConfig
-    AppConfig.reset()
-
 
 def test_sensitive_env_vars_masking(test_handler):
     logger, stream = test_handler