fix(openviking): read recall settings from config.yaml first, env vars as fallback - #62541
fix(openviking): read recall settings from config.yaml first, env vars as fallback#62541justemu wants to merge 2 commits into
Conversation
…s as fallback _recall_config() previously read all settings (recall_limit, score_threshold, recall_resources, etc.) exclusively from environment variables. This forced users to store behavioural configuration in .env, violating the Hermes convention that .env is for secrets only. The infrastructure to load config.yaml -> memory.openviking was already in place via _load_hermes_openviking_config(), but _recall_config() never called it. Fix: call _load_hermes_openviking_config() and pass its values as the default parameter to _env_int/_env_float/_env_bool. Env vars still override config.yaml values, preserving backward compatibility. Closes NousResearch#62540
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a confirmed configuration gap. Current main's plugins/memory/openviking/__init__.py:2587 reads recall policy exclusively through OPENVIKING_RECALL_*; the existing config loader at :855 is not used there.
Problems
- The changed config-resolution behavior is untested.
tests/openviking_plugin/test_openviking.py:343checks only defaults and schema entries, notmemory.openvikingvalues or the intended legacy env-var override.
Suggested changes
- Add a temp-
HERMES_HOMEtest that writesmemory.openvikingrecall settings inconfig.yaml, verifies_recall_config()consumes them, and verifies anOPENVIKING_RECALL_*value overrides config when present.
Automated hermes-sweeper review.
| @@ -2585,45 +2585,46 @@ def _env_float(name: str, default: float, *, minimum: float, maximum: float) -> | |||
| return max(minimum, min(maximum, value)) | |||
|
|
|||
| def _recall_config(self) -> Dict[str, Any]: | |||
| # Read from config.yaml → memory.openviking as primary source, env vars | |||
There was a problem hiding this comment.
Please add coverage for this new loader path using a temporary HERMES_HOME config.yaml, including the intended legacy environment-variable override. The existing recall-policy test only verifies built-in defaults.
…HOME tests Add three tests to TestOpenVikingConfigSchema: 1. test_recall_config_reads_from_config_yaml — writes memory.openviking settings in config.yaml and verifies _recall_config() consumes them. 2. test_recall_config_env_overrides_config_yaml — writes both config.yaml and OPENVIKING_RECALL_* env vars, verifies env takes precedence. 3. test_recall_config_partial_config_yaml — partially populated config.yaml falls back to defaults for omitted keys. All 46 openviking_plugin tests pass (43 existing + 3 new).
|
Addressed the sweeper's review:
|
|
Merged via #77747 — your commits cherry-picked with authorship preserved (rebase merge). This PR consolidated your recall settings from config.yaml fix together with 5 other OpenViking fixes into one coherent integration so the complete runtime behavior could be validated as a unit. Thanks for the contribution! |
Summary
OpenVikingMemoryProvider._recall_config()previously read all recall settings (recall_limit,score_threshold,recall_resources, etc.) exclusively from environment variables. This forced users to store behavioural configuration in.env, violating the Hermes convention that.envis for secrets only.Root Cause
The infrastructure to load
config.yaml → memory.openvikingwas already in place via_load_hermes_openviking_config(), and was used by_resolve_connection_settings()for connection parameters. But_recall_config()never called it — it only read env vars.Fix
_recall_config()now calls_load_hermes_openviking_config()and passes config.yaml values as thedefaultparameter to the existing_env_int/_env_float/_env_boolhelpers. Env vars still override config.yaml values, preserving backward compatibility.Priority
config.yaml → env var → built-in default. This matches how
_resolve_connection_settings()already uses config.yaml for connection parameters.Closes
Closes #62540