fix(rag): initialize the LLM service a RAG session creates - #742
fix(rag): initialize the LLM service a RAG session creates#742ayaangazali wants to merge 1 commit into
Conversation
RunanywhereAI#735 fixed this in HttpServer::loadModel and named the rule: rac_llm_create() only routes to the plugin's `create` op, and backends that defer the weight load to `initialize` (MLX) are not loaded when it returns. llama.cpp loads synchronously inside `create`, which hides the omission until another backend reaches `generate`. rac_rag_session_create_proto has the same shape. It calls rac_llm_create() for the session's generation model and hands the handle straight to RAGBackend, which drives it through rac_llm_generate at rag_pipeline_graph.cpp:85 and :357 and rac_llm_rerank's rag_rerank.cpp:121. Nothing between those points calls rac_llm_initialize, so a RAG session backed by MLX fails at generation with "model is not loaded" for exactly the reason the server did. The other two call sites in the tree already do this: llm_create_service (llm_module.cpp:335) has always paired create with initialize, and HttpServer now does after RunanywhereAI#735. This is the third. Failure handling matches the sibling: destroy the LLM handle, release the embedding service that was created just above, and publish the same rag.sessionCreate failure the other early-outs publish.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughRAG session creation now initializes the optional LLM service with its resolved model path. Initialization failures clean up both service handles, publish the error, and stop session creation. ChangesRAG session creation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change initializes LLM services created for RAG sessions, preventing deferred-load backends from failing at generation; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
What is wrong
#735 fixed this in
HttpServer::loadModeland stated the rule in its comment:rac_llm_create()only routes to the plugin'screateop, and a backend that defers the weight load toinitialize(MLX) is not loaded when it returns. llama.cpp loads synchronously insidecreate, which is why the omission stays invisible until another backend reachesgenerate.rac_rag_session_create_protohas the same shape. It creates the session's generation model:and hands that handle straight to
RAGBackend:rac_llm_initializedoes not appear anywhere inrac_rag_proto_abi.cpp. The handle is then driven directly through:rag_pipeline_graph.cpp:85(rac_llm_generate)rag_pipeline_graph.cpp:357(rac_llm_generate_stream)rag_rerank.cpp:121(rac_llm_generate)So a RAG session backed by MLX fails at generation with "model is not loaded", for exactly the reason the server did.
Why I am confident this is the same defect and not a deliberate difference
I checked every
rac_llm_createcall site in the tree. There are four, and the other three settle the question:rac_llm_initialize?llm_module.cpp:335(llm_create_service)http_server.cpp:309rac_rag_proto_abi.cpp:781core/tests/test_advanced_modality_proto_abi.cpp:1149What this does
Adds the
initializecall, with the same failure handling as the sibling early-out just above it: destroy the LLM handle, release the embedding service created a few lines earlier, and publish the samerag.sessionCreatefailure the other early-outs publish. 11 lines.Verification
Built and ran the full commons suite on this branch:
No test added, and I want to be straight about why: reproducing the failure needs a backend whose
createdefers the load, which in practice means MLX. The commons suite has no such backend, so a test here would either need a fake plugin built for this one case or would pass on llamacpp whether or not the fix is present. #735 added no test for the same reason. What I did instead was enumerate the call sites above, so the claim rests on the invariant the other three already follow rather than on a test that cannot distinguish the two states.Summary by CodeRabbit