Skip to content

feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260)#2

Merged
ddvnguyen merged 5 commits into
hydra-forkfrom
feat/m287-combined-expert-mode
Jun 19, 2026
Merged

feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260)#2
ddvnguyen merged 5 commits into
hydra-forkfrom
feat/m287-combined-expert-mode

Conversation

@ddvnguyen

@ddvnguyen ddvnguyen commented Jun 19, 2026

Copy link
Copy Markdown
Owner

Overview

Engine-side implementation of the COMBINED (expert-split) half of two-engine
"work together" — see hydra_vortex issue
#287 and epic ggml-org#161. The
coordinator side (Hydra.Core) already landed separately and is merged —
this PR is the llama.cpp-fork counterpart it expects.

A "head" engine can dual-load its routed-expert tensors onto a peer engine's
embedded ggml-RPC backend at startup, then flip per-request between the local
copy (SOLO) and the peer copy (COMBINED) via SET_EXPERT_MODE (0x44), with
transparent fallback to solo when the peer was never reachable.

PIPELINE (the prima.cpp-style layer-split half of ggml-org#287, opcode 0x46)
remains stubbed NOT_IMPLEMENTED — out of scope here, tracked as the
remaining half of ggml-org#287 (deliberately split: COMBINED is a finishable, lower-risk
slice; PIPELINE needs new graph-truncation work and a new wire opcode).

What changed

  • Role flag: --role standalone|head|worker (default standalone, zero
    behavior change for existing deployments).
    • --role worker --ggml-rpc-port <port>: the process becomes a pure
      embedded ggml_backend_rpc_start_server backend — no model load, no
      HTTP/control-RPC serving
      . This sidesteps running local inference and the
      embedded RPC server concurrently against the same GPU context, which is
      not a tested/supported pattern in upstream ggml.
    • --role head: reuses the existing --rpc-engine <peer host:port> flag
      and adds --combined-ot-pattern "<tensor-name regex>".
  • llama_hydra_load_combined_experts() (include/llama-hydra.h,
    src/llama-hydra.cpp): at head startup, after the model loads, copies every
    expert tensor whose name matches the pattern onto the peer's RPC backend
    buffer (chunked, 16 MB at a time, so multi-GB expert tensors never need a
    full in-memory staging buffer). The head's own local copies are untouched —
    SOLO always still works even if this fails.
  • llama_hydra_set_expert_mode() / get_expert_mode(): per-context
    toggle (llama_cparams.hydra_expert_mode) consumed by
    qwen35moe.cpp::build_layer_ffn(), which selects the dual-resident _rpc
    tensors instead of the local ones when COMBINED is active and they were
    actually loaded.
  • SET_EXPERT_MODE (0x44) now does real work instead of an
    unconditional ack: only honors "combined" when dual-load succeeded at
    startup (hydra_combined_capable), and reports the actual applied mode
    in the response ({"success":true,"mode":"solo"|"combined"}) so the
    Coordinator's ReportsSolo() can detect a fallback.
  • ENGINE_INFO (0x41) now reports
    role/mode/peer_connected/peer_addr/layer_split/combined_capable/pipeline_capable
    per hydra_vortex/specs/rpc-protocol.md.
  • specs/rpc-protocol.md (parent repo) updated to match what's actually
    implemented (the previous draft's 0x44 response shape and the ot_split
    syntax description were aspirational/stale).

Design notes / deviations worth flagging for review

  • Dual-residency is populated by a one-time network push, not true
    server-side local loading.
    The original design intent ("no weight
    transfer — the peer loads from its own local GGUF") would require a custom,
    non-stock RPC server protocol (stock ggml-rpc is a passive memory+compute
    server; there's no verb for "load this tensor yourself from disk"). That's
    a much larger change and is more naturally part of the PIPELINE half of
    In transcript mode the different the person(llama's emulations) the different the day of today. ggml-org/llama.cpp#287 if/when it's needed there too. This PR instead has the head push the
    matched tensor bytes to the peer once, at startup — not per-request, so
    steady-state inference still never moves weights over the wire, but the
    initial framing of "zero weight transfer, ever" doesn't quite hold. Flagging
    for discussion in case this isn't an acceptable tradeoff.
  • An earlier, never-finished spike (fixed color reset on exit ggml-org/llama.cpp#149/Error while converting to ggml.py format ggml-org/llama.cpp#260) had scaffolded an alternative
    ("Approach A": a post-graph-build hook that redirects mul_mat_id compute
    to a different backend without dual-residency). That doesn't actually give
    correct SOLO-fallback semantics — redirecting compute to a backend that
    doesn't hold the data either forces a per-call copy (defeating the point)
    or just doesn't work. This PR ports the spike's hydra_expert_mode cparam
    but drops that hook in favor of dual-residency ("Approach B"), which is what
    the original locked design (-ot "...=PEER", dual-resident) actually called
    for.

Build / test status

  • sm_120 (RTX, CUDA 13.2): clean build, llama-engine target, no warnings
    introduced. --role validation smoke-tested manually (invalid role and
    missing --ggml-rpc-port both fail with clear errors; see test commands
    below).
  • sm_60 (P100, CUDA 12.9): not independently built this pass — this
    change has no architecture-specific code, and both targets compile the same
    source through the same CMake path, so risk is low, but it hasn't been
    verified on real Pascal hardware.
  • Not yet tested: an actual two-process head+worker run on real RTX+P100
    hardware (dual-load succeeding, a real SET_EXPERT_MODE round trip,
    generation quality/perf with COMBINED active). That needs the deployed
    --ggml-rpc-port wired into infra/hydra-head/config/node-p100.yaml plus a
    live multi-GPU run — left for the deploy step once this merges (the
    Coordinator side already gates COMBINED behind a default-off config flag,
    so there's no risk to existing deployments from merging this as-is).
# build
cmake -B build_sm120 -DCMAKE_CUDA_COMPILER=/opt/software/cuda/13.2/bin/nvcc \
  -DCMAKE_CUDA_ARCHITECTURES=120 -DGGML_CUDA=ON -DGGML_CUDA_FORCE_CUBLAS=ON \
  -DGGML_RPC=ON -DGGML_NATIVE=ON -DCMAKE_BUILD_TYPE=Release -DLLAMA_BUILD_TESTS=OFF
cmake --build build_sm120 --target llama-engine -j$(nproc)

# smoke test
./build_sm120/bin/llama-engine --role bogus     # -> clean error, exit
./build_sm120/bin/llama-engine --role worker    # -> "requires --ggml-rpc-port"

Handoff / next steps for whoever picks up PIPELINE (ggml-org#287 remainder)

  • New wire opcode needed for steady-state head→worker activation forwarding
    during decode (proposed 0x47, not yet defined — 0x46 PIPELINE_ATTACH is
    attach-only).
  • Graph truncation in qwen35moe.cpp's graph::graph() constructor: head
    truncates the per-layer loop's upper bound and early-returns with
    res->t_embd as the boundary activation; worker truncates the lower bound
    and feeds the received activation in via the already-existing
    ubatch.embd raw-vector-input path (llm_graph_context::build_inp_embd,
    src/llama-graph.cpp) — no new graph-input plumbing needed there.
  • src/llama-model.cpp's create_memory() filter lambdas need a
    pipeline-split-aware layer-range gate for real KV-cache VRAM savings on
    both sides (currently only filters NextN/MTP layers).
  • Unlike COMBINED, PIPELINE's worker needs its own independent model load
    (own llama_context, own KV cache) — it should NOT also run the embedded
    ggml_backend_rpc_start_server thread this PR adds for COMBINED's worker
    role; that mechanism is COMBINED-specific.

AI usage disclosure: YES — this PR was written by Claude Code, with the
design validated against the actual ggml/llama.cpp source (graph-build
internals, RPC backend API, KV-cache layer filtering) rather than from
memory. A human (repo owner) should review before merge per AGENTS.md/CONTRIBUTING.md.

ddvnguyen and others added 5 commits June 19, 2026 14:20
M-Perf.9 (ggml-org#289) closes the loop on cross-model KV safety: a KV cache built
with model A is incompatible with model B, and the Coordinator must never
silently restore a cross-model cache. Three changes:

* llama_model_hash() — public API + impl that returns the SHA-256 of the
  GGUF file the model was loaded from. Computed once in
  llama_model_load_from_file after construction; stored on the
  llama_model struct as hash_hex. New self-contained SHA-256 helper in
  src/llama-hash.cpp (no OpenSSL/libcrypto).

* State META / GET now carry model_alias + model_hash + model_path so
  the Coordinator can record which model built each KV. STATE_META
  (0x32) and STATE_GET (0x30) responses populate these from
  impl->model_name / impl->params_base.model.path / llama_model_hash().

* Engine PREFILL (0x42) now accepts an optional 'model' key in the
  request JSON. The C++ side swaps to the requested alias when the
  preset registry knows it, falls back to the resident model with
  model_fallback: true in the response when the alias is unknown
  (or no preset is configured), and behaves as before when 'model' is
  absent. The engine uses the existing common_preset_context::load_from_ini
  (common/preset.cpp:315-365) — same INI format llama-server router mode
  already uses.

* Engine INFO (0x41) now advertises the new capabilities ('preset',
  'model_hash') and lists preset_aliases.

* Engine PIPELINE_ATTACH (0x46) is now defined as a real opcode
  (previously referenced in WIP that referenced 0x33-0x38 which collides
  with C# OpCode.GetManifest=0x33) and stubbed to return
  HYDRA_STATUS_NOT_IMPLEMENTED (new status 0x06) so the Coordinator can
  distinguish 'not yet built' (issue ggml-org#287) from a real error and fall
  back to solo mode. The dispatch in hydra_handle_connection routes
  0x46 to hydra_handle_pipeline_attach.

* server_task_result_hydra_state and server_task_result_hydra_engine
  gained the model_* fields; to_json() emits them.

Refs: ggml-org#289, advances ggml-org#287
Closes: M-Perf.9
…reset alias registry

Closes the 4 P0 review blockers from PR ggml-org#290.

P0.1 — Add the missing SERVER_TASK_TYPE_HYDRA_ENGINE_PIPELINE_ATTACH entry
        to the task enum. The PR's handler for 0x46 referenced an
        undefined symbol — the build would not have compiled.

P0.2 — Engine PREFILL now parses the optional 'model' key from the
        request body. When the alias is in the preset registry, the
        resident model is swapped via load_model() before the prefill
        runs; when the alias is unknown (or no preset is configured),
        the prefill runs on the resident model and the response carries
        model_fallback:true. The handler also re-derives the slot
        pointer after load_model since it allocates fresh slots.

P0.3 — Engine INFO now returns the actual preset_aliases list from
        the populated preset_alias_to_path map (was hardcoded empty).

P0.4 — Add preset_alias_to_path map to server_context_impl, populated
        at startup from --models-preset PATH (or directory of .ini
        files). Uses common_preset_context::load_from_ini and extracts
        LLAMA_ARG_MODEL from each preset section.

Also renumbered the dispatch cases from the old WIP
SERVER_TASK_TYPE_HYDRA_* names (0x33-0x38) to the new
SERVER_TASK_TYPE_HYDRA_ENGINE_* names (0x40-0x46). The old enum
entries are removed (the wire now exclusively uses 0x40-0x46 — the
0x33-0x38 range was a collision zone with the live C#
OpCode.GetManifest=0x33).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ookup sites

The original PR ggml-org#290 used 'model' as the variable name in 3 places
(STATE_GET, STATE_META, and the post-PREFILL response site). The
actual variable in server_context_impl is 'model_tgt' (declared at
server-context.cpp:680). The build failed with 'model was not
declared in this scope' at all 3 sites — caught by a fresh
sm_120 cmake build of llama-server.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…org#260)

Implements the engine-side half of issue ggml-org#287's COMBINED track: a head
engine can dual-load its routed-expert tensors onto a peer engine's
embedded ggml-RPC backend at startup, then flip per-request between the
local copy (SOLO) and the peer copy (COMBINED) via SET_EXPERT_MODE (0x44),
with transparent fallback to solo when the peer was never reachable.

- Role flag (--role standalone|head|worker, default standalone = no
  behavior change). --role worker makes the engine a pure embedded
  ggml-RPC backend server (no model load, no HTTP/control-RPC serving) on
  --ggml-rpc-port, sidestepping any concurrent local-inference-vs-RPC-server
  use of the same GPU context. --role head reuses the existing
  --rpc-engine <peer> flag and adds --combined-ot-pattern <regex>.
- llama_hydra_load_combined_experts() (include/llama-hydra.h,
  src/llama-hydra.cpp): one-time chunked network copy of every expert
  tensor matching the pattern onto the peer's RPC backend, run once at
  head startup. Local copies are untouched, so SOLO always still works.
- llama_hydra_set_expert_mode()/get_expert_mode(): per-context toggle
  (llama_cparams.hydra_expert_mode) consumed by qwen35moe.cpp's
  build_layer_ffn(), which selects the dual-resident _rpc tensors instead
  of the local ones when COMBINED is active and they were loaded.
- SET_EXPERT_MODE (0x44) handler now does real work instead of an
  unconditional ack: only honors "combined" when dual-load succeeded at
  startup, and reports the ACTUAL applied mode so the Coordinator's
  ReportsSolo() can detect the fallback.
- ENGINE_INFO (0x41) reports role/mode/peer_connected/peer_addr/
  layer_split/combined_capable/pipeline_capable per specs/rpc-protocol.md.

Superseded from an earlier, never-finished spike (ggml-org#149/ggml-org#260): this lands
the "manual init" the spike's own comments flagged as missing, on top of
the current llama-engine (ggml-org#261/ggml-org#262/ggml-org#289), not the old llama-server.

Deviation from the original "no weight transfer" framing: COMBINED's
dual-residency is populated by the head pushing tensor bytes to the peer
over ggml-RPC once at startup (not per-request) — true server-side local
loading would require a custom (non-stock) RPC server protocol, which is
out of scope here. PIPELINE (the prima.cpp-style layer-split track, 0x46)
remains stubbed NOT_IMPLEMENTED — tracked as the remaining half of ggml-org#287.

Verified: sm_120 (RTX) builds clean, llama-engine binary's --role
validation behaves as designed. sm_60 (P100) not independently built this
pass (no arch-specific code in this change; same toolchain/source path).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ddvnguyen

Copy link
Copy Markdown
Owner Author

Code review — #2 (feat/m287-combined-expert-mode → hydra-fork)

Cross-linked from ddvnguyen/hydra_vortex#293 (the parent-repo integration tracking issue; closes ddvnguyen/hydra_vortex#287 on the Hydra side once the parent submodule bumps to this PR's head).

Verdict

Ready to merge into hydra-fork. Both the M-Perf.9 model-identity work (also in #3) and the COMBINED-half of ggml-org#287 (the only half in this PR) are correctly implemented. The path is: merge this PR → close #3 as superseded → open a Hydra chore: bump src/llama-cpp submodule PR pointing at the new hydra-fork head.

What was reviewed

  • include/llama-hydra.h — new public API: llama_hydra_peer_reachable, llama_hydra_load_combined_experts, llama_hydra_set_expert_mode / _get_expert_mode
  • src/llama-hydra.cpp — ggml-RPC peer registration + chunked dual-load impl
  • src/llama-cparams.h / src/llama-context.{h,cpp} / src/llama-graph.hhydra_expert_mode cparam + graph-reuse key
  • src/llama-model.hffn_*_exps_rpc shadow pointers
  • src/models/qwen35moe.cpp:507-513 — SOLO/COMBINED tensor selection at graph-build time
  • tools/llama-engine/llama-engine.cpp — one-binary --role <standalone|head|worker> parser + worker role's embedded RPC server
  • tools/server/server-rpc.h + tools/server/server-task.{h,cpp} + tools/server/server-context.cpp — opcode 0x44 SET_EXPERT_MODE + INFO 0x41 reporting role/mode/capable/peer
  • Plus the full PR Feat/m perf 9 engine model identity #3 surface area inherited at base (5ef3b33fc1): llama-hash.cpp/h, llama_model_hash(), META/GET/PREFILL model-identity fields, opcode renumber 0x33-0x38 → 0x40-0x45, the 0x33-0x3F collision-zone comment in server-task.h:35-39

What's good

  • WIP commit 8baa00614f is honest. The hydra_placement_hook callback in process_ubatch was tried first, then reverted in favour of the cleaner _rpc shadow-tensor approach. No dead code in the final state — llama-context.cpp only has the hydra_set_expert_mode setter, the process_ubatch callback is gone, llama-context.h no longer carries hydra_placement_hook / hydra_set_placement_hook. Good discipline.
  • One-time startup cost is honest. The "no weight transfer" design applies to steady-state inference; llama_hydra_load_combined_experts is a chunked (16 MB) one-time copy at startup. The comment in llama-hydra.h:32-33 is accurate.
  • SOLO always works. The _rpc shadow pointers are nullptr when dual-load fails (peer unreachable, RPC backend missing, regex matches nothing), and qwen35moe.cpp:509 explicitly checks ffn_gate_exps_rpc != nullptr before selecting the rpc path. Failure modes are not silent.
  • SET_EXPERT_MODE silent-fallback is detectable. The new expert_mode_applied field on the result struct tells the Coordinator what the engine actually applied, distinct from what was requested. Coordinator can ReportsSolo() and never block a request on a half-built COMBINED path.
  • Graph-reuse key includes hydra_expert_mode in llama-graph.h:688. A mode change forces a rebuild, so SOLO/COMBINED transitions are not served by a stale graph using the wrong tensor set.
  • Worker role design is correct. run_combined_worker (llama-engine.cpp:184-226) deliberately never loads a model and never serves HTTP/RPC — it just embeds the ggml-RPC server and blocks. The comment at llama-engine.cpp:181-183 documents why local-inference + embedded-RPC on the same GPU context is not a supported upstream pattern.
  • Engine opcodes renumbered 0x33-0x38 → 0x40-0x45 with the 0x33-0x3F zone explicitly reserved (server-task.h:35-39). The reason (C# GetManifest=0x33) is documented inline. Good defensive design against future collision.
  • Self-contained SHA-256 in llama-hash.cpp — no OpenSSL/libcrypto. Adapts upstream examples/gguf-hash/gguf-hash.cpp correctly.
  • Engine PREFILL model swap uses common_preset_context::load_from_ini (the same INI format llama-server router mode already uses) and gracefully falls back to the resident model with model_fallback:true when the alias is unknown or no preset is configured. Doesn't block startup.

Cross-repo coordination gaps (minor, not blocking)

  1. C# EngineInfo doesn't read the new C++ fields. ddvnguyen/hydra_vortex src/core/Hydra.Core/Services/HydraEngineClient.cs deserializes only engine, version, capabilities, preset_aliases — it ignores the C++ side's role, mode, peer_connected, peer_addr, layer_split, combined_capable, pipeline_capable. The C# WorkerCapabilities class has these as config-side fields (from HYDRA_COORD_* env vars) but the engine-side self-report is informational, not verified. Not a bug for the current topology-comes-from-config design, but worth a follow-up issue: extend EngineInfo with EngineRole / EngineCombinedCapable and have the health poll cross-check against WorkerCapabilities.Role (mismatch → alert).
  2. C++ HYDRA_STATUS_NOT_IMPLEMENTED (0x06) is in server-rpc.h but not in the C# StatusCode enum. Works because the C# side reads the raw byte; enum drift.
  3. specs/rpc-protocol.md is out of date — it doesn't document opcodes 0x40-0x46 or the 0x33-0x3F collision zone. The C# Protocol.cs and C++ server-rpc.h are the actual sources of truth right now.

What's NOT in this PR (by design)

Open questions for the author

  • Cross-platform fork CI is failing on both PRs (ubuntu, windows, windows-arm, etc., all in mergeable_state: unstable). Owner decided to skip CI and trust local sm_120 / sm_60 builds — please run those before merging (DevelopmentRunBook.md "llama-server build", with the sm_60 explicit CUDA toolkit path) and confirm no new ggml-RPC symbol issues on the P100 build.
  • C# WorkerCapabilities.IsHead compares Role to "head" (case-insensitive) but the C++ engine field reports the value verbatim from --role (lowercased by extract_hydra_role_flags). The C# cross-check on IsHead should be fine, but if any other consumer parses engine.role, mind the case.
  • --models-preset INI discovery at server-context.cpp:1203-1228 tries file first, then scans the directory for *.ini. Is that ordering intentional? Some operators might point at a directory full of historical INIs and expect only the latest to load; the current code loads all of them. If multi-INI is intended, the doc string in server-context.cpp:1189-1193 should say so explicitly.

Recommended next steps

  1. Local cmake --build build_sm120 --target llama-server + cmake --build build_sm60 --target llama-server pass.
  2. Merge this PR into hydra-fork (fast-forward: c357ad25b8 will become the new hydra-fork head).
  3. Close Feat/m perf 9 engine model identity #3 as superseded.
  4. Open ddvnguyen/hydra_vortex PR chore: bump src/llama-cpp submodule to c357ad25b with cross-links to both fork PRs in the body.
  5. Push-before-PR guard: git ls-remote https://github.com/ddvnguyen/llama.cpp refs/heads/hydra-fork | grep c357ad25b8 returns a hit (it will, since this is the merge commit).
  6. Smoke test the new opcodes (META, PREFILL, SET_EXPERT_MODE) against the live system per docs/workflow/06-monitoring.md.
  7. Update specs/rpc-protocol.md to document 0x40-0x46 + the 0x33-0x3F zone.
  8. File the three follow-up issues (C# EngineInfo read-side, StatusCode.NotImplemented enum, C++ unit tests).

@ddvnguyen ddvnguyen changed the title Feat/m287 combined expert mode feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260) Jun 19, 2026
@ddvnguyen

Copy link
Copy Markdown
Owner Author

Re-review — ddvnguyen/llama.cpp#2 (after PR #3 merged into hydra-fork)

Re-reviewing per the maintainer note. No new code on this PR — the 5 commits and 20 files (+972/-39) are byte-identical to the version I reviewed 7 hours ago. The "update" is in the base branch: PR #3 (M-Perf.9 model identity) has been merged into hydra-fork at 0e667e1961d82f5b9b330821c5827e80f6feb775 (a non-fast-forward merge).

Divergence

gh api repos/ddvnguyen/llama.cpp/compare/...:

  • feat/m287-combined-expert-mode is 2 commits ahead and 1 commit behind hydra-fork.
  • Status: diverged.
  • The 3 M-Perf.9 commits at the PR's base (1c755a0da9 / 8b8198fdca / 5ef3b33fc1) are content-equivalent to the tree at 0e667e1961 — I confirmed byte-equivalence on src/llama-hash.cpp via diff and via git diff-tree (the only tree differences are in tools/server/server-context.cpp and tools/server/server-task.h, which were the post-alaways "failed to tokenize string! " ggml-org/llama.cpp#290 follow-up edits in the PR's later commits). So the M-Perf.9 work exists on both branches in slightly different shapes.

Verdict (unchanged from my earlier review)

Ready to merge after a rebase — the code-level review still holds. The implementation is correct, the cross-repo coordination gaps I noted earlier are minor and not blocking, and the PIPELINE half of ggml-org#287 remains a separate deliverable.

Recommended path forward

Rebase feat/m287-combined-expert-mode onto hydra-fork so the 3 M-Perf.9 commits collapse (they're now in hydra-fork via the merge) and the PR becomes a clean 2-commit fast-forward:

cd src/llama-cpp
git fetch ddvnguyen hydra-fork
git checkout feat/m287-combined-expert-mode
git rebase ddvnguyen/hydra-fork
# If conflicts surface in server-task.h / server-context.cpp (where the
# post-#290 follow-up edits were), the resolution is to keep the
# PR's version — it has the P1+P2 fixes layered on top of M-Perf.9.
git push --force-with-lease ddvnguyen feat/m287-combined-expert-mode

After the rebase the branch is just 8baa00614f (wip: port hydra_expert_mode placement-hook scaffolding — partial) and c357ad25b8 (feat: COMBINED expert-split two-engine mode) on top of the new hydra-fork tip. Merging that into hydra-fork is then a fast-forward with no merge commit.

Then in the parent repo (ddvnguyen/hydra_vortex):

  1. The submodule pointer bumps from 5ef3b33fc to the new hydra-fork tip (the fast-forward successor of 0e667e1961).
  2. PR [WIP] Improve performance on x86 ggml-org/llama.cpp#295 (chore: bump src/llama-cpp submodule — COMBINED expert-split engine mode (part of #287)) is the candidate — but its current pointer 5ef3b33fc → c357ad25b will be wrong after the rebase (the rebased head will be a new commit, not c357ad25b). PR [WIP] Improve performance on x86 ggml-org/llama.cpp#295 needs a force-push / repoint to the rebased head before it can pass the new 08-llama-fork.md push-before-PR guard.
  3. My new PR (#296feat: cross-model guard metrics + mix-precision P/D split semantics) drops the cross-quantization prefill/decode model names from workers.json (interpretation (b): same model, different worker choice). This is independent of the fork PR but relevant context — the COMBINED mode work in this fork PR can co-exist with the interpretation-(b) Hydra config; they're orthogonal.

Re-review of the three "open questions for the author" from my earlier review

  • Cross-platform CI: still in mergeable_state: unstable. Fork Actions are now disabled at the repo level (per the maintainer's earlier request), so the failing jobs are stale; they will not run again unless re-enabled. Local sm_120 / sm_60 builds are the only gate. Worth running those before merging.
  • C# WorkerCapabilities.IsHead case-sensitivity: still applies. Not blocking. Out of scope for this fork PR.
  • --models-preset INI discovery order: still applies. Not blocking. A doc comment in server-context.cpp:1189-1193 clarifying the multi-INI intent would be nice-to-have.

Three new follow-up issues that emerged from this re-review

These are not blockers for this PR but worth filing:

  1. PR Feat/m perf 9 engine model identity #3 needs to be closed as superseded once PR feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260) #2 merges (PR Feat/m perf 9 engine model identity #3 is the strict subset; PR feat: COMBINED expert-split two-engine mode (hydra_vortex issue #287/#260) #2 contains PR Feat/m perf 9 engine model identity #3 + the COMBINED work). This is the maintainer's job.
  2. PR [WIP] Improve performance on x86 ggml-org/llama.cpp#295's submodule pointer needs a follow-up push to point at the rebased hydra-fork head (a new SHA, not c357ad25b). Either the author of [WIP] Improve performance on x86 ggml-org/llama.cpp#295 rebases [WIP] Improve performance on x86 ggml-org/llama.cpp#295's branch on top of the new submodule commit, or PR [WIP] Improve performance on x86 ggml-org/llama.cpp#295 gets closed and a new one is opened with the right pointer.
  3. C# EngineInfo should be extended to read the new C++ engine fields (role, mode, combined_capable, pipeline_capable, peer_connected, peer_addr, layer_split) so the engine-side self-report is cross-checked against WorkerCapabilities.Role in the health poll. Out of scope for this PR; small follow-up in the parent repo. (#296 already includes the Skip/Proceed metrics for the cross-model guard, but the C# EngineInfo reader-side is still pending.)

TL;DR

  • Code review verdict: still ready to merge, no new findings.
  • Action: rebase the branch onto the new hydra-fork tip (drop the 3 M-Perf.9 commits that are now in hydra-fork via the merge commit), then fast-forward merge.
  • Action (parent repo side): update PR [WIP] Improve performance on x86 ggml-org/llama.cpp#295's submodule pointer to the post-rebase SHA, or close and reopen.
  • Blockers on integration: none from this PR's code. The blockers are sequencing (the rebase hasn't happened yet) and the new submodule pointer (needs a follow-up push on [WIP] Improve performance on x86 ggml-org/llama.cpp#295 or a new PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant