You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review of merged PR #48 ("fork: hydra_config inline T3 + hydra_metrics response") found the following non-blocking cleanup/altitude findings. Companion to the P0/P1 correctness issue (#49) — see that issue for the severe bugs; this one is quality-only.
P2-7 — Altitude: the inline HTTP bootstrap duplicates the existing CONFIGURE(0x40) mechanism
The proper fix for "can't do a first load via CONFIGURE" is removing the && ctx_tgt guard in process_single_task() (server-context.cpp:3259,3265) — which is now unnecessary since #48 already made the T3 statics (s_hydra_pending_* in llama-hydra.cpp) ctx-independent by design — and enqueuing a CONFIGURE task instead of adding a second synchronous HTTP-thread path in post_chat_completions.
The existing, documented (specs/rpc-protocol.md) binary CONFIGURE mechanism already does this job properly: it respects the slot-free-moment gate via update_slots(), and has a real response envelope (tier/params_applied/deferred_keys). Routing the bootstrap through it instead would eliminate P0-3, P0-4, and P1-5 from #49 as a side effect (single code path, already queue-mediated, no new undocumented wire fields).
Also: the new hydra_config request field and hydra_metrics/t3_reloaded/t3_reload_ms response fields are undocumented in specs/rpc-protocol.md — the canonical wire-format doc — despite duplicating functionality that is documented there for the 0x40 CONFIGURE path.
P2-8 — Cleanup batch: dead code, duplicated parsers, wasted copies
Dead function: llama_hydra_apply_pending_config() (src/llama-hydra.cpp:552, exported in include/llama-hydra.h:38) — a ~55-line function — is never called anywhere in the codebase. A parallel, independently-written copy of the same drain-timeout logic lives in server-context.cpp:4047's apply_pending_hydra_config(). Either wire the exported one up or delete it; keeping both means future drain-timeout changes have to remember to patch two places.
Duplicated + weaker validation: hydra_parse_cache_type() (server-context.cpp:4118) reimplements — and is looser than — the existing kv_cache_type_from_str() in common/arg.cpp:402 (already used for the server's own -ctk/-ctv flags). The existing helper restricts matches to 7 curated valid KV-cache types; the new one accepts any of ~40 ggml types, silently letting CONFIGURE set a KV cache type that was never validated as KV-cache-safe.
Duplicated parser (root cause of [P0/P1] hydra_config inline T3 bootstrap: unreachable path, dangling pointer, meta never refreshed, thread race #49's P0-2): apply_t3_rebuild()'s override_tensor comma/equals parsing loop duplicates common/arg.cpp:247's parse_tensor_buffer_overrides() almost line for line, including rebuilding the same buft_list map — and it's the duplicate copy, not the original, that has the dangling-pointer bug. Reusing the existing helper would have sidestepped the bug entirely.
Unreachable block: a second "model" key-handling block in the CONFIGURE handler (server-context.cpp:3211-3225) is dead — hydra_classify_config_key("model") already tiers and defers it earlier in the same function (3179-3197). The block only exists to guard against re-duplicating work it doesn't need to do (via a manual std::find scan on deferred_keys).
Unnecessary JSON copy/re-serialize: t1_subset/t2t3_subset (server-context.cpp:3176-3225) are rebuilt key-by-key from cfg, then t2t3_subset.dump()'d again — even though the consumers (hydra_apply_t1_config, hydra_apply_t3_mutators) only ever do .contains() lookups and would behave identically against cfg directly.
Wasted copy: get_meta() (server-context.cpp:5788) unconditionally copies all 128 entries of params_base.tensor_split on every call. post_chat_completions's own metrics code immediately re-trims trailing zeros at the call site right after — that trim belongs in get_meta() once, not redone by every caller.
Filed from a code review requested against Hydra's COMBINED-mode architecture. See linked summary issue in ddvnguyen/hydra_vortex for board tracking.
Review of merged PR #48 ("fork: hydra_config inline T3 + hydra_metrics response") found the following non-blocking cleanup/altitude findings. Companion to the P0/P1 correctness issue (#49) — see that issue for the severe bugs; this one is quality-only.
Hydra parent: ddvnguyen/hydra_vortex#410 / ggml-org#411.
P2-7 — Altitude: the inline HTTP bootstrap duplicates the existing CONFIGURE(0x40) mechanism
The proper fix for "can't do a first load via CONFIGURE" is removing the
&& ctx_tgtguard inprocess_single_task()(server-context.cpp:3259,3265) — which is now unnecessary since #48 already made the T3 statics (s_hydra_pending_*inllama-hydra.cpp) ctx-independent by design — and enqueuing a CONFIGURE task instead of adding a second synchronous HTTP-thread path inpost_chat_completions.The existing, documented (
specs/rpc-protocol.md) binary CONFIGURE mechanism already does this job properly: it respects the slot-free-moment gate viaupdate_slots(), and has a real response envelope (tier/params_applied/deferred_keys). Routing the bootstrap through it instead would eliminate P0-3, P0-4, and P1-5 from #49 as a side effect (single code path, already queue-mediated, no new undocumented wire fields).Also: the new
hydra_configrequest field andhydra_metrics/t3_reloaded/t3_reload_msresponse fields are undocumented inspecs/rpc-protocol.md— the canonical wire-format doc — despite duplicating functionality that is documented there for the 0x40 CONFIGURE path.P2-8 — Cleanup batch: dead code, duplicated parsers, wasted copies
Dead function:
llama_hydra_apply_pending_config()(src/llama-hydra.cpp:552, exported ininclude/llama-hydra.h:38) — a ~55-line function — is never called anywhere in the codebase. A parallel, independently-written copy of the same drain-timeout logic lives inserver-context.cpp:4047'sapply_pending_hydra_config(). Either wire the exported one up or delete it; keeping both means future drain-timeout changes have to remember to patch two places.Duplicated + weaker validation:
hydra_parse_cache_type()(server-context.cpp:4118) reimplements — and is looser than — the existingkv_cache_type_from_str()incommon/arg.cpp:402(already used for the server's own-ctk/-ctvflags). The existing helper restricts matches to 7 curated valid KV-cache types; the new one accepts any of ~40 ggml types, silently letting CONFIGURE set a KV cache type that was never validated as KV-cache-safe.Duplicated parser (root cause of [P0/P1] hydra_config inline T3 bootstrap: unreachable path, dangling pointer, meta never refreshed, thread race #49's P0-2):
apply_t3_rebuild()'soverride_tensorcomma/equals parsing loop duplicatescommon/arg.cpp:247'sparse_tensor_buffer_overrides()almost line for line, including rebuilding the samebuft_listmap — and it's the duplicate copy, not the original, that has the dangling-pointer bug. Reusing the existing helper would have sidestepped the bug entirely.Unreachable block: a second "model" key-handling block in the CONFIGURE handler (
server-context.cpp:3211-3225) is dead —hydra_classify_config_key("model")already tiers and defers it earlier in the same function (3179-3197). The block only exists to guard against re-duplicating work it doesn't need to do (via a manualstd::findscan ondeferred_keys).Unnecessary JSON copy/re-serialize:
t1_subset/t2t3_subset(server-context.cpp:3176-3225) are rebuilt key-by-key fromcfg, thent2t3_subset.dump()'d again — even though the consumers (hydra_apply_t1_config,hydra_apply_t3_mutators) only ever do.contains()lookups and would behave identically againstcfgdirectly.Wasted copy:
get_meta()(server-context.cpp:5788) unconditionally copies all 128 entries ofparams_base.tensor_spliton every call.post_chat_completions's own metrics code immediately re-trims trailing zeros at the call site right after — that trim belongs inget_meta()once, not redone by every caller.Filed from a code review requested against Hydra's COMBINED-mode architecture. See linked summary issue in ddvnguyen/hydra_vortex for board tracking.