All-Hands-AI / OpenHands

🙌 OpenHands: Code Less, Make More
https://all-hands.dev
MIT License
36.6k stars 4.15k forks source link

[Feature]: Make location of configuration file configurable #3947

Closed neubig closed 1 month ago

neubig commented 1 month ago

What problem or use case are you trying to solve?

Currently OpenHands, by default, loads config.toml from the base directory of the repository. However, if we have a single OpenHands installation that is used by multiple users, or we wanted to use OpenHands as a library, this behavior may be too rigid.

Describe the UX of the solution you'd like

It should be possible to specify the location of the config.toml file by adding an argument to the load_app_config() function in openhands/core/config/app_config.py, and then modify all command line programs that call load_app_config() so that they take in an argument that can optionally specify the location of the config.toml file.

rehanganapathy commented 1 month ago

hey there, this looks like something i can work on, what would i have to do to get this assigned to me?

neubig commented 1 month ago

Thanks for volunteering! I assigned the issue to you

rehanganapathy commented 1 month ago

will work on this thank you!

tobitege commented 1 month ago

@neubig should we more closely specify how this arg is being coming in, i.e. which name of env variable or similar? 🤔

rehanganapathy commented 1 month ago

would love to have the specifics

tobitege commented 1 month ago

@rehanganapathy - here's a tidbit I had asked Sonnet to do for me, maybe you can incorporate that (if it seems correct)? All within the file openhands/core/config/utils.py

Refactored load_from_toml Method

def load_from_toml(cfg: AppConfig, toml_file: str = 'config.toml'):
    """Load the config from the toml file. Supports both styles of config vars.

    Args:
        cfg: The AppConfig object to update attributes of.
        toml_file: The path to the toml file. Defaults to 'config.toml'.
    """
    toml_config = read_toml_file(toml_file)
    if toml_config is None:
        return

    if 'core' not in toml_config:
        load_from_env(cfg, toml_config)
        return

    process_core_config(cfg, toml_config['core'])
    load_nested_configs(cfg, toml_config)
    migrate_sandbox_configs(cfg, toml_config.get('core', {}))
    finalize_toml_loading(cfg, toml_config)

Added Helper Functions

Read TOML File

def read_toml_file(toml_file: str) -> dict | None:
    """Read and parse the TOML configuration file.

    Args:
        toml_file: The path to the toml file.

    Returns:
        Parsed TOML configuration as a dictionary or None if an error occurs.
    """
    try:
        with open(toml_file, 'r', encoding='utf-8') as file:
            return toml.load(file)
    except FileNotFoundError:
        return None
    except toml.TomlDecodeError as e:
        logger.openhands_logger.warning(
            f'Cannot parse config from toml, toml values have not been applied.\nError: {e}',
            exc_info=False,
        )
        return None

Process Core Configuration

def process_core_config(cfg: AppConfig, core_config: dict):
    """Process the core section of the TOML configuration.

    Args:
        cfg: The AppConfig object to update.
        core_config: The core section of the TOML configuration.
    """
    for key, value in core_config.items():
        if key.startswith('sandbox_'):
            new_key = key.replace('sandbox_', '')
            if new_key in cfg.sandbox.__annotations__:
                setattr(cfg.sandbox, new_key, value)
            else:
                logger.openhands_logger.warning(f'Unknown sandbox config: {key}')
        elif hasattr(cfg, key):
            setattr(cfg, key, value)
        else:
            logger.openhands_logger.warning(f'Unknown core config key: {key}')

Load Nested Configurations

def load_nested_configs(cfg: AppConfig, toml_config: dict):
    """Load nested configurations such as 'agent' and 'llm'.

    Args:
        cfg: The AppConfig object to update.
        toml_config: The entire TOML configuration.
    """
    for key, value in toml_config.items():
        if isinstance(value, dict):
            if key.lower() == 'agent':
                load_agent_config(cfg, value)
            elif key.lower() == 'llm':
                load_llm_config(cfg, value)
            elif key.lower() != 'core' and not key.startswith('sandbox'):
                logger.openhands_logger.warning(f'Unknown key in config.toml: "{key}"')

Load Agent Configuration

def load_agent_config(cfg: AppConfig, agent_values: dict):
    """Load agent configurations from the TOML file.

    Args:
        cfg: The AppConfig object to update.
        agent_values: The agent section of the TOML configuration.
    """
    try:
        non_dict_fields = {k: v for k, v in agent_values.items() if not isinstance(v, dict)}
        agent_config = AgentConfig(**non_dict_fields)
        cfg.set_agent_config(agent_config, 'agent')
        for nested_key, nested_value in agent_values.items():
            if isinstance(nested_value, dict):
                nested_agent_config = AgentConfig(**nested_value)
                cfg.set_agent_config(nested_agent_config, nested_key)
    except (TypeError, KeyError) as e:
        logger.openhands_logger.warning(
            f'Cannot parse agent config from toml.\nError: {e}',
            exc_info=False,
        )

Load LLM Configuration

def load_llm_config(cfg: AppConfig, llm_values: dict):
    """Load LLM configurations from the TOML file.

    Args:
        cfg: The AppConfig object to update.
        llm_values: The llm section of the TOML configuration.
    """
    try:
        non_dict_fields = {k: v for k, v in llm_values.items() if not isinstance(v, dict)}
        llm_config = LLMConfig(**non_dict_fields)
        cfg.set_llm_config(llm_config, 'llm')
        for nested_key, nested_value in llm_values.items():
            if isinstance(nested_value, dict):
                nested_llm_config = LLMConfig(**nested_value)
                cfg.set_llm_config(nested_llm_config, nested_key)
    except (TypeError, KeyError) as e:
        logger.openhands_logger.warning(
            f'Cannot parse llm config from toml.\nError: {e}',
            exc_info=False,
        )

Migrate Sandbox Configurations

def migrate_sandbox_configs(cfg: AppConfig, core_config: dict):
    """Migrate old sandbox configurations from the core section.

    Args:
        cfg: The AppConfig object to update.
        core_config: The core section of the TOML configuration.
    """
    sandbox_config = cfg.sandbox
    keys_to_migrate = [key for key in core_config if key.startswith('sandbox_')]
    for key in keys_to_migrate:
        new_key = key.replace('sandbox_', '')
        if new_key in sandbox_config.__annotations__:
            setattr(sandbox_config, new_key, core_config.pop(key))
        else:
            logger.openhands_logger.warning(f'Unknown sandbox config: {key}')

Finalize TOML Loading

def finalize_toml_loading(cfg: AppConfig, toml_config: dict):
    """Finalize the loading process by handling the sandbox and core configurations.

    Args:
        cfg: The AppConfig object to update.
        toml_config: The entire TOML configuration.
    """
    try:
        if 'sandbox' in toml_config:
            cfg.sandbox = SandboxConfig(**toml_config['sandbox'])

        for key, value in toml_config.get('core', {}).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 finalize config from toml.\nError: {e}',
            exc_info=False,
        )

Summary of Changes

  1. Extracted TOML Reading: Created read_toml_file to handle file reading and error handling.
  2. Processed Core Config Separately: Moved core configuration processing to process_core_config for clarity.
  3. Loaded Nested Configurations: Split loading of nested agent and llm configurations into load_nested_configs, load_agent_config, and load_llm_config.
  4. Migrated Sandbox Configurations: Isolated sandbox migration logic into migrate_sandbox_configs.
  5. Finalized Loading Process: Consolidated final steps into finalize_toml_loading to handle sandbox and core configurations post-loading.

This refactoring enhances readability by modularizing the load_from_toml method into smaller, focused functions, making the code easier to maintain and understand.

rehanganapathy commented 1 month ago

right thank you, will work with this

neubig commented 1 month ago

Hey @rehanganapathy , please tell us if you have any further questions!

rehanganapathy commented 1 month ago

hey there, sorry its taking too long, will try finishing this asap, thank you!

neubig commented 1 month ago

Thanks!

rehanganapathy commented 1 month ago

please check it out: https://github.com/All-Hands-AI/OpenHands/pull/4168

enyst commented 1 month ago

Thank you @rehanganapathy ! Setting this as solved.