refactor(config): migrate from .env to YAML with OmegaConf#350
refactor(config): migrate from .env to YAML with OmegaConf#350looksaw2 wants to merge 3 commits into
Conversation
Replace pydantic-settings + python-dotenv with OmegaConf-based config.yaml. Supports nested YAML structure, environment variable override (os.environ > config.yaml > defaults), auto-migration from existing .env, and hot-reload via file watcher. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Update design.md to reflect the nested YAML structure (via flat↔nested mapping), update tasks.md to mark all 10 task groups completed, and document implementation differences from the original plan. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Translate requirements.md, design.md, and tasks.md from Chinese to English to meet project contribution guidelines. Co-Authored-By: Claude Opus 4.7 <[email protected]>
| # 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 |
There was a problem hiding this comment.
This loop copies every key from .env into os.environ before YAML/env override resolution. That reverses the intended deployment precedence when the process already has real environment variables: I reproduced this with .env containing OPENAI_API_KEY=from_dotenv while launching with OPENAI_API_KEY=from_real_env, and llm_settings.openai_chat_api_key became from_dotenv. Existing pydantic settings gave the real environment precedence over the dotenv file, so a stale local .env can override Docker/Kubernetes secrets after this migration. Please only fill missing keys from .env (for example os.environ.setdefault(...), skipping empty values) and add a regression test for env > .env/config.yaml precedence.
| os.environ[k] = v | |
| for k, v in dotenv_values(self._env_path).items(): | |
| if v: | |
| os.environ.setdefault(k, v) |
VGalaxies
left a comment
There was a problem hiding this comment.
Review summary
- Blocking: yes
- Summary: The YAML migration introduces config persistence and compatibility regressions that can break existing deployments.
- Evidence:
- static review of
git diff origin/main...HEAD git diff --check origin/main...HEADclean- no changed files under
hugegraph-llm/src/tests/
- static review of
| current, | ||
| ) | ||
| setattr(self, key, value) | ||
| cfg_mgr.update_section(self._config_section, self) |
There was a problem hiding this 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_mtimebeforereload()at lines 260-264, thenreload()callscheck_config(), which always savesconfig.yamlagain 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_mtimeafter internal saves and suppress self-triggered reloads.
| } | ||
|
|
||
| _env_var_map: ClassVar[dict] = { | ||
| "openai_chat_api_key": "OPENAI_API_KEY", |
There was a problem hiding this comment.
Medium: Legacy provider-specific OpenAI environment variables are ignored
hugegraph-llm/src/hugegraph_llm/config/llm_config.py:75
Evidence
_env_var_mapmapsopenai_chat_api_key,openai_extract_api_key, andopenai_text2gql_api_keyonly toOPENAI_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 onceconfig.yamlexists.
Requested fix
- Preserve field-specific env names and use generic
OPENAI_API_KEY/OPENAI_BASE_URLonly as fallback, with tests covering both paths.
|
|
||
| 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") |
There was a problem hiding this comment.
Medium: Config files are resolved from the process CWD
hugegraph-llm/src/hugegraph_llm/config/models/base_config.py:32
Evidence
YAML_PATHandENV_PATHare hardcoded toos.getcwd(), while the source setup inhugegraph-llm/README.mdruns from the repository root and existing docs/Docker examples refer tohugegraph-llm/.env.
Impact
- Launching from the repository root skips migration of an existing
hugegraph-llm/.envand creates/uses a different root-levelconfig.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-llmproject root, or add an explicit config path override and update all launch/migration paths consistently.
Migrated config storage from .env to config.yaml using OmegaConf.
/ admin / index sections with nested structure.
restarting. Polling interval is configurable.
first run, no manual setup needed.
environment variables, overriding YAML values (12-factor friendly).
exactly the same, 46 consumer files untouched.