VGalaxies commented on code in PR #350:
URL: https://github.com/apache/hugegraph-ai/pull/350#discussion_r3371594896
##########
hugegraph-llm/src/hugegraph_llm/config/llm_config.py:
##########
@@ -16,15 +16,73 @@
# under the License.
-import os
-from typing import Literal, Optional
+from typing import ClassVar, Literal, Optional
from .models import BaseConfig
class LLMConfig(BaseConfig):
"""LLM settings"""
+ _config_section: ClassVar[str] = "llm"
+
+ _flat_to_nested_mapping: ClassVar[dict] = {
+ "openai_chat_api_base": "openai.chat.api_base",
+ "openai_chat_api_key": "openai.chat.api_key",
+ "openai_chat_language_model": "openai.chat.language_model",
+ "openai_chat_tokens": "openai.chat.tokens",
+ "openai_extract_api_base": "openai.extract.api_base",
+ "openai_extract_api_key": "openai.extract.api_key",
+ "openai_extract_language_model": "openai.extract.language_model",
+ "openai_extract_tokens": "openai.extract.tokens",
+ "openai_text2gql_api_base": "openai.text2gql.api_base",
+ "openai_text2gql_api_key": "openai.text2gql.api_key",
+ "openai_text2gql_language_model": "openai.text2gql.language_model",
+ "openai_text2gql_tokens": "openai.text2gql.tokens",
+ "openai_embedding_api_base": "openai.embedding.api_base",
+ "openai_embedding_api_key": "openai.embedding.api_key",
+ "openai_embedding_model": "openai.embedding.model",
+ "ollama_chat_host": "ollama.chat.host",
+ "ollama_chat_port": "ollama.chat.port",
+ "ollama_chat_language_model": "ollama.chat.language_model",
+ "ollama_extract_host": "ollama.extract.host",
+ "ollama_extract_port": "ollama.extract.port",
+ "ollama_extract_language_model": "ollama.extract.language_model",
+ "ollama_text2gql_host": "ollama.text2gql.host",
+ "ollama_text2gql_port": "ollama.text2gql.port",
+ "ollama_text2gql_language_model": "ollama.text2gql.language_model",
+ "ollama_embedding_host": "ollama.embedding.host",
+ "ollama_embedding_port": "ollama.embedding.port",
+ "ollama_embedding_model": "ollama.embedding.model",
+ "litellm_chat_api_key": "litellm.chat.api_key",
+ "litellm_chat_api_base": "litellm.chat.api_base",
+ "litellm_chat_language_model": "litellm.chat.language_model",
+ "litellm_chat_tokens": "litellm.chat.tokens",
+ "litellm_extract_api_key": "litellm.extract.api_key",
+ "litellm_extract_api_base": "litellm.extract.api_base",
+ "litellm_extract_language_model": "litellm.extract.language_model",
+ "litellm_extract_tokens": "litellm.extract.tokens",
+ "litellm_text2gql_api_key": "litellm.text2gql.api_key",
+ "litellm_text2gql_api_base": "litellm.text2gql.api_base",
+ "litellm_text2gql_language_model": "litellm.text2gql.language_model",
+ "litellm_text2gql_tokens": "litellm.text2gql.tokens",
+ "litellm_embedding_api_key": "litellm.embedding.api_key",
+ "litellm_embedding_api_base": "litellm.embedding.api_base",
+ "litellm_embedding_model": "litellm.embedding.model",
+ }
+
+ _env_var_map: ClassVar[dict] = {
+ "openai_chat_api_key": "OPENAI_API_KEY",
Review Comment:
**Medium: Legacy provider-specific OpenAI environment variables are ignored**
`hugegraph-llm/src/hugegraph_llm/config/llm_config.py:75`
**Evidence**
- `_env_var_map` maps `openai_chat_api_key`, `openai_extract_api_key`, and
`openai_text2gql_api_key` only to `OPENAI_API_KEY`; the override loop checks
only that mapped name instead of also checking the field’s own uppercase name.
**Impact**
- Existing deployments using `OPENAI_CHAT_API_KEY`,
`OPENAI_EXTRACT_API_KEY`, `OPENAI_TEXT2GQL_API_KEY`, or matching per-provider
base URL variables stop overriding config once `config.yaml` exists.
**Requested fix**
- Preserve field-specific env names and use generic `OPENAI_API_KEY` /
`OPENAI_BASE_URL` only as fallback, with tests covering both paths.
##########
hugegraph-llm/src/hugegraph_llm/config/models/base_config.py:
##########
@@ -16,130 +16,342 @@
# under the License.
+import collections.abc
import os
+import threading
+import time
+from typing import ClassVar, Optional
-from dotenv import dotenv_values, set_key
-from pydantic_settings import BaseSettings
+from dotenv import dotenv_values
+from omegaconf import DictConfig, OmegaConf
+from pydantic import BaseModel, ConfigDict, TypeAdapter
from hugegraph_llm.utils.log import log
dir_name = os.path.dirname
-env_path = os.path.join(os.getcwd(), ".env") # Load .env from the current
working directory
+YAML_PATH = os.path.join(os.getcwd(), "config.yaml")
Review Comment:
**Medium: Config files are resolved from the process CWD**
`hugegraph-llm/src/hugegraph_llm/config/models/base_config.py:32`
**Evidence**
- `YAML_PATH` and `ENV_PATH` are hardcoded to `os.getcwd()`, while the
source setup in `hugegraph-llm/README.md` runs from the repository root and
existing docs/Docker examples refer to `hugegraph-llm/.env`.
**Impact**
- Launching from the repository root skips migration of an existing
`hugegraph-llm/.env` and creates/uses a different root-level `config.yaml`, so
users can silently run with defaults instead of their configured credentials
and graph settings.
**Requested fix**
- Resolve the default config path from the `hugegraph-llm` project root, or
add an explicit config path override and update all launch/migration paths
consistently.
##########
hugegraph-llm/src/hugegraph_llm/config/models/base_config.py:
##########
@@ -16,130 +16,342 @@
# under the License.
+import collections.abc
import os
+import threading
+import time
+from typing import ClassVar, Optional
-from dotenv import dotenv_values, set_key
-from pydantic_settings import BaseSettings
+from dotenv import dotenv_values
+from omegaconf import DictConfig, OmegaConf
+from pydantic import BaseModel, ConfigDict, TypeAdapter
from hugegraph_llm.utils.log import log
dir_name = os.path.dirname
-env_path = os.path.join(os.getcwd(), ".env") # Load .env from the current
working directory
+YAML_PATH = os.path.join(os.getcwd(), "config.yaml")
+ENV_PATH = os.path.join(os.getcwd(), ".env")
-class BaseConfig(BaseSettings):
- class Config:
- env_file = env_path
- case_sensitive = False
- extra = "ignore" # ignore extra fields to avoid ValidationError
- env_ignore_empty = True
+def _flat_to_nested(flat_dict: dict, mapping: dict) -> dict:
+ """Convert flat field names to nested dict using dot-notation mapping.
- def generate_env(self):
- if os.path.exists(env_path):
+ Mapping: {"flat_name": "nested.path.key", ...}
+ Fields not in the mapping are kept at the top level.
+ """
+ if not mapping:
+ return flat_dict
+ result: dict = {}
+ for field_name, value in flat_dict.items():
+ if field_name in mapping:
+ path = mapping[field_name]
+ parts = path.split(".")
+ d = result
+ for part in parts[:-1]:
+ if part not in d:
+ d[part] = {}
+ d = d[part]
+ d[parts[-1]] = value
+ else:
+ result[field_name] = value
+ return result
+
+
+def _nested_to_flat(nested_dict: dict, mapping: dict) -> dict:
+ """Convert nested dict from YAML to flat field names using dot-notation
mapping.
+
+ Reverse of _flat_to_nested. Walks the nested dict, matching dot-joined
+ paths against the mapping keys.
+ """
+ if not mapping or not nested_dict:
+ return nested_dict
+ reverse_map = {v: k for k, v in mapping.items()}
+ result = {}
+
+ def _walk(prefix: str, d: dict) -> None:
+ for key, value in d.items():
+ full_key = f"{prefix}.{key}" if prefix else key
+ if full_key in reverse_map:
+ result[reverse_map[full_key]] = value
+ elif isinstance(value, collections.abc.Mapping):
+ _walk(full_key, value)
+ else:
+ result[full_key.replace(".", "_")] = value
+
+ _walk("", nested_dict)
+ return result
+
+
+class ConfigManager:
+ """Singleton manager for OmegaConf-based YAML configuration.
+
+ Lifecycle:
+ 1. __init__: load config.yaml or migrate from .env, start file watcher
+ 2. Config classes read via get_section_with_env_override()
+ 3. Config classes write via update_section() + save()
+ 4. Background watcher polls for external changes → reload()
+ """
+
+ _instance: ClassVar[Optional["ConfigManager"]] = None
+
+ def __new__(cls, sections=None):
+ if cls._instance is None:
+ cls._instance = super().__new__(cls)
+ cls._instance._initialized = False
+ return cls._instance
+
+ def __init__(self, sections=None):
+ if self._initialized:
+ return
+ self._initialized = True
+ self._yaml_path = YAML_PATH
+ self._env_path = ENV_PATH
+ self._sections: dict = sections or {}
+ self._cfg: DictConfig = OmegaConf.create({})
+ self._reload_lock = threading.Lock()
+ self._watching = False
+ self._watcher_thread: Optional[threading.Thread] = None
+ self._last_mtime: float = 0.0
+ self._reload_targets: list = [] # (section_name, config_object) tuples
+
+ # Load .env into os.environ for backward compatibility and priority
override
+ if os.path.exists(self._env_path):
+ for k, v in dotenv_values(self._env_path).items():
+ os.environ[k] = v
+
+ # Load or migrate
+ if os.path.exists(self._yaml_path):
+ self._cfg = OmegaConf.load(self._yaml_path)
+ log.info("Loaded config from %s", self._yaml_path)
+ elif os.path.exists(self._env_path):
+ self._cfg = self._migrate_from_env()
+ else:
+ self._cfg = OmegaConf.create({})
+ log.info("No config file found, using defaults")
+
+ self._start_file_watcher()
+
+ def _migrate_from_env(self) -> DictConfig:
+ """Migrate .env to config.yaml with type conversion and nested
structure.
+
+ Only fields present in .env are written to YAML.
+ """
+ env_data = dotenv_values(self._env_path)
+ cfg = OmegaConf.create({})
+ for section_name, model_class in self._sections.items():
+ model_fields = set(model_class.model_fields.keys())
+ mapping = getattr(model_class, "_flat_to_nested_mapping", {})
+ section_data = {}
+ for env_key, env_value in env_data.items():
+ lower_key = env_key.lower()
+ if lower_key in model_fields and env_value:
+ section_data[lower_key] = env_value
+ if section_data:
+ instance = model_class(**section_data)
+ full_dump = instance.model_dump()
+ filtered = {k: v for k, v in full_dump.items() if k in
section_data}
+ nested = _flat_to_nested(filtered, mapping)
+ cfg[section_name] = OmegaConf.create(nested)
+ else:
+ cfg[section_name] = OmegaConf.create({})
+ OmegaConf.save(cfg, self._yaml_path)
+ log.info("Migrated %s to %s", self._env_path, self._yaml_path)
+ return cfg
+
+ def load(self) -> DictConfig:
+ """Re-read config from disk."""
+ return OmegaConf.load(self._yaml_path)
+
+ def save(self) -> None:
+ """Persist current config tree to disk."""
+ OmegaConf.save(self._cfg, self._yaml_path)
+
+ def get_section(self, name: str) -> DictConfig:
+ """Get a config section, creating it if it does not exist."""
+ if name not in self._cfg:
+ self._cfg[name] = OmegaConf.create({})
+ return self._cfg[name]
+
+ def get_section_with_env_override(self, section_name: str, model_class:
type) -> dict:
+ """Load section from YAML, convert nested→flat, then override with env
vars.
+
+ Priority: os.environ > config.yaml > pydantic defaults.
+ """
+ yaml_section = self.get_section(section_name)
+ raw_dict = OmegaConf.to_container(yaml_section, resolve=True) if
yaml_section else {}
+ if raw_dict is None:
+ raw_dict = {}
+
+ mapping = getattr(model_class, "_flat_to_nested_mapping", {})
+ section_dict = _nested_to_flat(raw_dict, mapping)
+
+ env_var_map: dict = getattr(model_class, "_env_var_map", {})
+
+ for field_name in model_class.model_fields:
+ env_var_name = env_var_map.get(field_name, field_name.upper())
+ env_value = os.environ.get(env_var_name)
+ if env_value is not None and env_value != "":
+ try:
+ field_info = model_class.model_fields[field_name]
+ ta = TypeAdapter(field_info.annotation)
+ section_dict[field_name] = ta.validate_python(env_value)
+ except Exception:
+ section_dict[field_name] = env_value
+
+ return section_dict
+
+ def update_section(self, name: str, model: BaseModel) -> None:
+ """Sync pydantic model fields into OmegaConf section with nested
structure."""
+ model_dict = model.model_dump()
+ mapping = getattr(type(model), "_flat_to_nested_mapping", {})
+ nested_dict = _flat_to_nested(model_dict, mapping)
+ if name not in self._cfg:
+ self._cfg[name] = OmegaConf.create({})
+ self._cfg[name] = OmegaConf.create(nested_dict)
+
+ def register_reload_target(self, section_name: str, config_object:
BaseModel) -> None:
+ """Register a config object for automatic sync on hot-reload."""
+ self._reload_targets.append((section_name, config_object))
+
+ def reload(self) -> bool:
+ """Hot-reload config from YAML, validate, and sync to registered
objects.
+
+ Returns True on success. On failure, keeps current in-memory config.
+ """
+ with self._reload_lock:
+ try:
+ new_cfg = OmegaConf.load(self._yaml_path)
+ for section_name, model_class in self._sections.items():
+ if section_name in new_cfg:
+ raw_dict =
OmegaConf.to_container(new_cfg[section_name], resolve=True)
+ if raw_dict:
+ mapping = getattr(model_class,
"_flat_to_nested_mapping", {})
+ flat_dict = _nested_to_flat(raw_dict, mapping)
+ model_class(**flat_dict)
+ self._cfg = new_cfg
+ for section_name, config_obj in self._reload_targets:
+ try:
+ config_obj.check_config()
+ except Exception as e:
+ log.error("Failed to sync '%s' on reload: %s",
section_name, e)
+ log.info("Config reloaded from %s", self._yaml_path)
+ return True
+ except Exception as e:
+ log.error("Failed to reload config: %s. Keeping current
values.", e)
+ return False
+
+ def _start_file_watcher(self) -> None:
+ """Start background daemon thread polling for config file changes."""
+ admin_section = self.get_section("admin")
+ interval = admin_section.get("config_reload_interval", 5) if
admin_section else 5
+ if not isinstance(interval, (int, float)) or interval <= 0:
+ log.info("Config hot-reload disabled (config_reload_interval=%s)",
interval)
+ return
+
+ self._watching = True
+ self._last_mtime = os.path.getmtime(self._yaml_path) if
os.path.exists(self._yaml_path) else 0.0
+
+ def _watch_loop():
+ while self._watching:
+ time.sleep(interval)
+ try:
+ if not os.path.exists(self._yaml_path):
+ continue
+ current_mtime = os.path.getmtime(self._yaml_path)
+ if current_mtime != self._last_mtime:
+ self._last_mtime = current_mtime
+ log.info("Config file change detected, reloading...")
+ self.reload()
+ except Exception as e:
+ log.error("File watcher error: %s", e)
+
+ self._watcher_thread = threading.Thread(target=_watch_loop,
daemon=True)
+ self._watcher_thread.start()
+ log.info("Config file watcher started (interval=%ss)", interval)
+
+ def _stop_file_watcher(self) -> None:
+ """Stop the background file watcher thread."""
+ self._watching = False
+
+
+class BaseConfig(BaseModel):
+ """Base configuration class using OmegaConf/YAML for persistence.
+
+ Subclasses must define:
+ - _config_section: ClassVar[str] — YAML section name ("llm", "hugegraph",
etc.)
+ - _flat_to_nested_mapping: ClassVar[dict] — flat field → nested path
mapping
+ """
+
+ model_config = ConfigDict(extra="ignore")
+
+ _config_section: ClassVar[str] = ""
+ _flat_to_nested_mapping: ClassVar[dict] = {}
+
+ def __init__(self, **data):
+ cfg_mgr = ConfigManager()
+ yaml_data = {}
+ if self._config_section:
+ yaml_data =
cfg_mgr.get_section_with_env_override(self._config_section, type(self))
+ yaml_data.update(data)
+ super().__init__(**yaml_data)
+ if self._config_section:
+ cfg_mgr.update_section(self._config_section, self)
+ cfg_mgr.save()
+ cfg_mgr.register_reload_target(self._config_section, self)
+ log.info("Config section '%s' initialized.", self._config_section)
+
+ def update_config(self) -> None:
+ """Persist current pydantic field values to config.yaml."""
+ cfg_mgr = ConfigManager()
+ cfg_mgr.update_section(self._config_section, self)
+ cfg_mgr.save()
+ log.info("Config section '%s' updated and saved.",
self._config_section)
+
+ def generate_yaml(self) -> None:
+ """Generate config.yaml section with current model default values."""
+ cfg_mgr = ConfigManager()
+ if os.path.exists(YAML_PATH):
log.info(
"%s already exists, do you want to override with the default
configuration? (y/n)",
- env_path,
+ YAML_PATH,
)
update = input()
if update.lower() != "y":
return
- self.update_env()
- else:
- config_dict = self.model_dump()
- config_dict = {k.upper(): v for k, v in config_dict.items()}
- with open(env_path, "w", encoding="utf-8") as f:
- for k, v in config_dict.items():
- if v is None:
- f.write(f"{k}=\n")
- else:
- f.write(f"{k}={v}\n")
- log.info("Generate %s successfully!", env_path)
-
- def update_env(self):
- config_dict = self.model_dump()
- config_dict = {k.upper(): v for k, v in config_dict.items()}
- env_config = dotenv_values(f"{env_path}")
-
- # dotenv_values make None to '', while pydantic make None to None
- # dotenv_values make integer to string, while pydantic make integer to
integer
- for k, v in config_dict.items():
- if k in env_config:
- if not (env_config[k] or v):
- continue
- if env_config[k] == str(v):
- continue
- log.info("Update %s: %s=%s", env_path, k, v)
- set_key(env_path, k, v if v else "", quote_mode="never")
-
- def check_env(self):
- """Synchronize configs between .env file and object.
-
- This method performs two steps:
- 1. Updates object attributes from .env file values when they differ
- 2. Adds missing configuration items to the .env file
- """
+ cfg_mgr.update_section(self._config_section, self)
+ cfg_mgr.save()
+ log.info("Generated %s section '%s' successfully!", YAML_PATH,
self._config_section)
+
+ def check_config(self) -> None:
+ """Synchronize config from YAML file to object attributes."""
+ cfg_mgr = ConfigManager()
try:
- # Read the.env file and prepare object config
- env_config = dotenv_values(env_path)
- config_dict = {k.upper(): v for k, v in self.model_dump().items()}
-
- # Step 1: Update the object from .env when values differ
- self._sync_env_to_object(env_config, config_dict)
- # Step 2: Add missing config items to .env
- self._sync_object_to_env(env_config, config_dict)
+ yaml_data =
cfg_mgr.get_section_with_env_override(self._config_section, type(self))
+ for key, value in yaml_data.items():
+ current = getattr(self, key, None)
+ if current != value:
+ log.info(
+ "Update configuration from file: %s=%s (was: %s)",
+ key,
+ value,
+ current,
+ )
+ setattr(self, key, value)
+ cfg_mgr.update_section(self._config_section, self)
Review Comment:
**High: Hot reload rewrites the watched file repeatedly**
`hugegraph-llm/src/hugegraph_llm/config/models/base_config.py:340`
**Evidence**
- The watcher records `_last_mtime` before `reload()` at lines 260-264, then
`reload()` calls `check_config()`, which always saves `config.yaml` again at
lines 340-341.
**Impact**
- After startup or any external edit, the internal save changes mtime again,
so the watcher can reload and rewrite the file every interval indefinitely,
causing repeated disk writes and log noise.
**Requested fix**
- Do not save from the hot-reload sync path, or update `_last_mtime` after
internal saves and suppress self-triggered reloads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
