perf: use load_config_readonly() at read-only call sites in agent/ - #56085
perf: use load_config_readonly() at read-only call sites in agent/#56085Stoltemberg wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the cached-config deepcopy cost; the performance premise still holds on current main.
Problems
- Blocking:
agent/agent_runtime_helpers.py:1722changes_sm_cfgtoload_config_readonly()and passes it toget_compatible_custom_providers(). Currenthermes_cli/config.py:4699-4700and:4714-4722mutate provider-entry dictionaries in place, whileload_config_readonly()explicitly forbids nested mutation (hermes_cli/config.py:6766-6781). This can corrupt the shared config cache for camelCase orapi_key_envprovider configurations. - The cleanup is now incomplete: current read-only-only paths remain in
agent/auxiliary_client.py:2175-2181,:2196-2202, andagent/coding_context.py:369-376.
Suggested changes
- First make the provider normalizer operate on a copied entry (and add a regression test that alias normalization does not mutate its input), then re-audit the remaining current-HEAD call sites during conflict resolution.
Automated hermes-sweeper review.
| from hermes_cli.config import load_config, get_compatible_custom_providers | ||
| _sm_cfg = load_config() | ||
| from hermes_cli.config import load_config_readonly, get_compatible_custom_providers | ||
| _sm_cfg = load_config_readonly() |
There was a problem hiding this comment.
load_config_readonly() returns the shared cached dict, but this value is passed to get_compatible_custom_providers(). On current main, its normalizer writes alias keys into provider-entry dicts (hermes_cli/config.py:4699-4700, :4714-4722), so this path can corrupt the config cache. Please make that normalizer/caller non-mutating before using the readonly loader here.
Salvaged from #56085 (@Stoltemberg), rebased onto current main: sites main had already converted (credential_pool, auxiliary_client MoA paths, model_metadata, moa_loop, agent_runtime_helpers) resolve to main's versions; the remaining ~29 read-only sites across 16 agent/ files swap to the no-deepcopy readonly loader (~135us saved per call). Full per-site mutation audit performed (every enclosing function read, escapes traced): 23 SAFE, 5 ESCAPES with read-only consumers, 1 UNSAFE path (init_agent -> get_compatible_custom_providers -> normalizer in-place alias writes) fixed by the preceding no-mutate commits, which make the normalizer copy-safe for ALL callers.
|
Thanks @Stoltemberg — this landed on main via #74322 with your commit preserved as author. Salvage notes: rebased onto current main (several of your sites had been independently converted in the meantime and resolve to main's versions; ~29 survived), and a full per-site mutation audit found one path where the swap was unsafe — init_agent → get_compatible_custom_providers → _normalize_custom_provider_entry mutated cached provider sub-dicts in place. That normalizer bug was fixed first (via @golldyck's #57096) so your swaps are safe everywhere. Measured 332µs → 11.9µs per read (28x) on a real config. Second salvaged PR of yours today (#55176 → #74194) — appreciate the consistent, well-aimed perf work! |
The terminal-command guard path loaded config 2-3x per invocation via load_config(), which pays a defensive deepcopy of the entire config on every call (~356us of the ~376us warm-cache cost measured on a real config.yaml). All six swapped call sites were audited read-only — every caller takes scalar reads or iterates the returned structures; none mutate (the save path at save_permanent_allowlist keeps load_config) — so they now use load_config_readonly(), the API built for exactly this (precedent: NousResearch#74211, NousResearch#74322; the one unsafe-site lesson from NousResearch#56085's salvage is covered by the mutation audit and a cache-integrity test). Measured (real config.yaml, warm cache): load_config 376.0us -> load_config_readonly 19.9us (18.9x); full guard pass check_all_command_guards('ls -la','local') 930.7us -> 241.8us (3.85x). Tests: new test_approval_config_readonly.py drives the real functions against a temp HERMES_HOME — readonly call counts per function, a no-deepcopy pin for the full guard pass, and cache-identity/integrity checks. Existing test mocks retargeted from load_config to load_config_readonly (same injection intent). Note: 6 test_approval_mode_parity failures are pre-existing ordering flakes — identical with the change stashed on clean main.
The terminal-command guard path loaded config 2-3x per invocation via load_config(), which pays a defensive deepcopy of the entire config on every call (~356us of the ~376us warm-cache cost measured on a real config.yaml). All six swapped call sites were audited read-only — every caller takes scalar reads or iterates the returned structures; none mutate (the save path at save_permanent_allowlist keeps load_config) — so they now use load_config_readonly(), the API built for exactly this (precedent: #74211, #74322; the one unsafe-site lesson from #56085's salvage is covered by the mutation audit and a cache-integrity test). Measured (real config.yaml, warm cache): load_config 376.0us -> load_config_readonly 19.9us (18.9x); full guard pass check_all_command_guards('ls -la','local') 930.7us -> 241.8us (3.85x). Tests: new test_approval_config_readonly.py drives the real functions against a temp HERMES_HOME — readonly call counts per function, a no-deepcopy pin for the full guard pass, and cache-identity/integrity checks. Existing test mocks retargeted from load_config to load_config_readonly (same injection intent). Note: 6 test_approval_mode_parity failures are pre-existing ordering flakes — identical with the change stashed on clean main.
Salvaged from NousResearch#56085 (@Stoltemberg), rebased onto current main: sites main had already converted (credential_pool, auxiliary_client MoA paths, model_metadata, moa_loop, agent_runtime_helpers) resolve to main's versions; the remaining ~29 read-only sites across 16 agent/ files swap to the no-deepcopy readonly loader (~135us saved per call). Full per-site mutation audit performed (every enclosing function read, escapes traced): 23 SAFE, 5 ESCAPES with read-only consumers, 1 UNSAFE path (init_agent -> get_compatible_custom_providers -> normalizer in-place alias writes) fixed by the preceding no-mutate commits, which make the normalizer copy-safe for ALL callers.
The terminal-command guard path loaded config 2-3x per invocation via load_config(), which pays a defensive deepcopy of the entire config on every call (~356us of the ~376us warm-cache cost measured on a real config.yaml). All six swapped call sites were audited read-only — every caller takes scalar reads or iterates the returned structures; none mutate (the save path at save_permanent_allowlist keeps load_config) — so they now use load_config_readonly(), the API built for exactly this (precedent: NousResearch#74211, NousResearch#74322; the one unsafe-site lesson from NousResearch#56085's salvage is covered by the mutation audit and a cache-integrity test). Measured (real config.yaml, warm cache): load_config 376.0us -> load_config_readonly 19.9us (18.9x); full guard pass check_all_command_guards('ls -la','local') 930.7us -> 241.8us (3.85x). Tests: new test_approval_config_readonly.py drives the real functions against a temp HERMES_HOME — readonly call counts per function, a no-deepcopy pin for the full guard pass, and cache-identity/integrity checks. Existing test mocks retargeted from load_config to load_config_readonly (same injection intent). Note: 6 test_approval_mode_parity failures are pre-existing ordering flakes — identical with the change stashed on clean main.
Salvaged from NousResearch#56085 (@Stoltemberg), rebased onto current main: sites main had already converted (credential_pool, auxiliary_client MoA paths, model_metadata, moa_loop, agent_runtime_helpers) resolve to main's versions; the remaining ~29 read-only sites across 16 agent/ files swap to the no-deepcopy readonly loader (~135us saved per call). Full per-site mutation audit performed (every enclosing function read, escapes traced): 23 SAFE, 5 ESCAPES with read-only consumers, 1 UNSAFE path (init_agent -> get_compatible_custom_providers -> normalizer in-place alias writes) fixed by the preceding no-mutate commits, which make the normalizer copy-safe for ALL callers.
What does this PR do?
load_config()performs a defensive deepcopy (~135μs per call) of the cached config dict. The codebase hasload_config_readonly()specifically for read-only callers, but most call sites inagent/still usedload_config().Replaced
load_config()→load_config_readonly()at 31+ read-only call sites across 20 files inagent/. Each saves ~135μs per call. In the agent loop, config is read 20-50x per conversation.Type of Change
Changes Made
20 files in
agent/— all confirmed read-only (only.get()lookups, no mutation):agent_init.py(4 aliased imports),auxiliary_client.py(11 calls),curator.py(2),prompt_builder.py(2), and 16 other files (1 each)How to Test
Checklist