docs: core public API design — phased implementation plan#44
Conversation
a994ccc to
59f1ae4
Compare
|
@ashwing Thank you for the job on this PR , it clarifies general functions required in the core module howeverI have a concern with this is that the design implementations are based on agent assumptions on some input output which is not explicit and once the actual implementation is in place the design might not be working well and developer would need to make so much changes and the review process would become complex. |
|
Fair point — stubs that drift from implementation would just create rework. I'll shift the approach: instead of landing all the type definitions at once, I'll implement one complete slice end-to-end (e.g., The design doc stays as a reference for the overall direction, but the PRs will be small, tested features — one step function at a time with integration tests proving the interfaces work. |
59f1ae4 to
1232f6e
Compare
|
Updated — stripped the code stubs and kept this as a pure design doc for inline review. Implementation will follow as separate small PRs with integration tests against the store layer. @leseb @franciscojavierarceo @maralbahari would appreciate your review on the design direction in |
d92b158 to
3501d67
Compare
Thank you @ashwing for the proposal. I have a simple response/conversation flow in #46. this PR only implements the stateful agent loop for simple input text messages it might look big because of the integration tests and cassettes records against OpenAI's conversation and responses api for assessment. However the agentic-loop is not complete with function_calls, instructions the default tool calls as your proposal which contains a bigger skeleton to ship. as discussed on slack we can work on #46 to have a more explicit implementation and complete the agentic-loop. I think could start with implementation to complete the function_calls and handling reasoning tokens etc? |
|
@maralbahari sounds good — I reviewed PR #46 and left some comments there. Makes sense for you to own the stateful text flow as the base, and I'll build function_calls, tool dispatch, and the looped execution on top once #46 lands. The design doc here stays as the reference for the broader skeleton. |
Consolidated proposal incorporating leseb's expanded 14-function vision with phased implementation plan. Intended for inline PR review per @franciscojavierarceo's request. Ref: vllm-project#42 Signed-off-by: Ashwin Giridharan <girida@amazon.com>
…dation Reframes the design doc as a hybrid reference: - Acknowledges PR vllm-project#46 (maralbahari) as the base executor loop - Defines clear ownership boundaries (base loop vs tool dispatch) - Organizes into 4 implementation phases, each = one PR - Phase 1 (SSE events) independent of PR vllm-project#46 - Phases 2-4 build on top of PR vllm-project#46 Removes speculative API surface (AgenticState, AgenticConfig, full trait definitions) in favor of concrete code snippets matching actual implementation targets. Keeps just enough detail to execute follow-up PRs without over-specifying. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
3501d67 to
bc210e5
Compare
- Phase 1 correctly depends on PR vllm-project#46 (accumulator.rs lives there) - call_inference is sync fn returning lazy stream, not async - persist_response takes explicit handler params (noted) - Native async traits instead of #[async_trait] (Rust 1.85) - Removed undefined ContextSize type, use &str - Phase 2 explicitly non-streaming (streaming gated on Phase 3) - Removed max_iterations redundancy from dispatch_tools params - ADR-01 reference reworded as paraphrase not quote Signed-off-by: Ashwin Giridharan <girida@amazon.com>
|
@franciscojavierarceo @maralbahari @leseb Gentle nudge to review the updated implementation plan. The doc is now minimal with work items phased out. |
|
|
||
| ### Phase 4: Tool Executor Traits + Mock Implementations (depends on Phase 2) | ||
|
|
||
| **PR scope:** `executor/tools/` module. |
There was a problem hiding this comment.
@ashwing I think for the tools it should be on just agentic-core/tools as ADR-03
we need to keep the modules in agentic-core as small as possible to avoid circular imports and dependencies so it is easier to maintain as well.
There was a problem hiding this comment.
@ashwing just nit the tools would be in agentic-core/tools not executor/tools
There was a problem hiding this comment.
Makes sense — keeps tool traits at the crate top level rather than coupling them to the executor. crates/agentic-core/src/tools/ it is. The executor just imports and calls them.
|
|
||
| ### Phase 3: Streaming Tee (depends on PR #46) | ||
|
|
||
| **PR scope:** `executor/stream_tee.rs`, refactor `run_stream` path. |
There was a problem hiding this comment.
@ashwing I think as SSE streaming grows we can also maintain them in their own small module in agentic-core/sse.
|
|
||
| ## Open Questions | ||
|
|
||
| 1. **`execute_loop` vs refactoring `execute`:** Should the loop wrapper be a new function or replace PR #46's `execute()`? Pending maralbahari's response on PR #46 review. |
There was a problem hiding this comment.
the execute in #46 is temporary as the entire loop is not complete with whole functionality since it is only validating text-only responses. so that we can test each small component and verify their correctness as we implement each composable pieces.
we shall write the last execute_loop once we have all the pieces in.
Thank you @ashwing I have responded to your questions in #46 (comment) |
Phase 1 now targets a separate `events/` module in agentic-core per @maralbahari's feedback on PR vllm-project#46 — avoids bloating the accumulator and has no PR vllm-project#46 dependency (lands on main directly). Also adds OGX compatibility note to Phase 4 traits and updates the Praxis filter mapping with the event normalizer step. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
|
|
||
| This PR includes mock implementations for integration testing (in-memory tool executors that return canned responses). Real implementations (MCP client, Brave search, Qdrant) come in later PRs. | ||
|
|
||
| **Note:** These traits must be compatible with @franciscojavierarceo's OGX integration (PR #34). The trait-based approach allows OGX to be one implementation behind `McpToolExecutor` — the dispatch layer doesn't care whether tools run via OGX or native Rust. |
There was a problem hiding this comment.
@ashwing Can you clarify what you mean here? OGX is a vector store search service—the existing integration (vector_search/ogx.rs) implements a VectorSearch trait and hits OGX's REST API directly. It's not an MCP server.
Did you mean OGX would be an implementation behind VectorStoreClient (not McpToolExecutor)? That would make sense and is basically what we already have.
If you're proposing routing OGX through MCP as an intermediary, I'd push back on that—it adds indirection for something that's a straightforward REST call.
There was a problem hiding this comment.
You're right — that was sloppy wording on my part. OGX is a VectorStoreClient implementation, not MCP. The dispatch layer routes by tool type: function calls → McpToolExecutor, file_search → VectorStoreClient (your OGX impl), web_search → WebSearchProvider. No MCP indirection for direct REST calls.
Won't push a fix since you've already approved — the correct mapping is in the Praxis filter table below and will be reflected in the code PRs.
| | 0 | `request_validate` | `validate_request()` | Future | — | | ||
| | 1 | `response_store` (init) | `init_store()` | Future | — | | ||
| | 2 | `rehydrate` | `rehydrate_conversation()` | PR #46 | @maralbahari | | ||
| | 3 | `file_resolve` | `resolve_files()` | Future | — | |
There was a problem hiding this comment.
happy to take this one on
| | 1 | `response_store` (init) | `init_store()` | Future | — | | ||
| | 2 | `rehydrate` | `rehydrate_conversation()` | PR #46 | @maralbahari | | ||
| | 3 | `file_resolve` | `resolve_files()` | Future | — | | ||
| | 4 | `tool_parse` | `parse_tools()` | Future | — | |
| | 8 | `mcp_tool` | `McpToolExecutor::execute()` | Phase 4 | @ashwing | | ||
| | 9 | `web_search` | `WebSearchProvider::search()` | Phase 4 | @ashwing | | ||
| | 10 | `file_search` | `VectorStoreClient::search()` | Phase 4 | @ashwing | | ||
| | 11 | `compact` | `compact_context()` | Future | — | |
| | 9 | `web_search` | `WebSearchProvider::search()` | Phase 4 | @ashwing | | ||
| | 10 | `file_search` | `VectorStoreClient::search()` | Phase 4 | @ashwing | | ||
| | 11 | `compact` | `compact_context()` | Future | — | | ||
| | 12 | `reasoning` | `summarize_reasoning()` | Future | — | |
|
|
||
| | # | Praxis Filter | Core Function | Phase | Owner | | ||
| |---|---------------|---------------|-------|-------| | ||
| | 0 | `request_validate` | `validate_request()` | Future | — | |
There was a problem hiding this comment.
i can do this one too
There was a problem hiding this comment.
Awesome — updated ownership split in my head:
You:
file_resolve/resolve_files()tool_parse/parse_tools()web_search/WebSearchProvider(Brave or similar)file_search/VectorStoreClient(OGX)
Me:
event_normalize(PR feat: add SSE event normalizer module #49 — submitted)stream_events/ streaming teetool_dispatch/dispatch_tools()+LoopDecisionmcp_tool/McpToolExecutor
The dispatch layer calls your providers via the trait interface so we can develop in parallel once trait definitions are agreed. Let me know if you want to define the trait signatures for your pieces or if you'd rather I propose them in a follow-up PR and you iterate.
franciscojavierarceo
left a comment
There was a problem hiding this comment.
some small nits but otherwise this lgtm
Signed-off-by: Ashwin Giridharan <girida@amazon.com>
|
@maralbahari @franciscojavierarceo Updated — tools path, OGX mapping, and ownership table all reflect your feedback. Thanks! |
Summary
Design reference for the
agentic-corepublic API extensions. This doc defines what we're building on top of PR #46's base executor loop.Ownership split:
execute(),rehydrate_conversation(),call_inference(),persist_response()(PR feat:agentic-coreconversation/responses hydration (ADR-03) #46)Implementation phases (each = one PR):
LoopDecision+dispatch_tools()+execute_loop()What changed in this update:
agentic-coreconversation/responses hydration (ADR-03) #46 as the foundationTest Plan
Design doc only — no code changes. Verification via follow-up implementation PRs with integration tests.