Repository navigation
feat(trace): replay Dynamo agent request traces - #3
ishandhanani wants to merge 13 commits into
Conversation
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@18da1715635681940a4a7dd676d263f8b493a777Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@18da1715635681940a4a7dd676d263f8b493a777Last updated for commit: |
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
ajcasagrande
left a comment
There was a problem hiding this comment.
Reviewed idhanani/agentx-dynamo-trajectories @ e99a179d9 vs ajc/agentx. Strong, clean refactor — moving Dynamo conversation-aware routing from body-injected nvext.session_control to X-Dynamo-Trajectory-ID / X-Dynamo-Parent-Trajectory-ID headers. No correctness defects found. Validated against source and reproduced end-to-end with the in-repo mock server.
What's working well
- Dead-symbol removal is complete — no dangling references to the removed flags/helpers/detector anywhere in
src/,docs/, ortests/. get_endpoint_headers()returns a fresh dict per call, so the.update(...)ininference_client.pycan't leak headers across requests.synthesize dynamo-traceoutput validates against the realWekaTraceschema and round-trips through the loader; subagent inner-requesttis emitted in absolute root-trace coordinates, exactly asweka_trace.py::_subagent_request_absolute_texpects.- Verified E2E against the mock server: every turn carries a stable trajectory header with zero
nvextin the body; fixed-schedule Weka replay dispatches children through branch orchestration only (no double-dispatch), with correct parent lineage. 155 unit tests + the DAG integration test pass on this branch.
Main thing to resolve: merge ordering (not a code defect)
This removes the legacy open/bind/close lifecycle entirely and depends on ai-dynamo/dynamo#10214. Deployments without #10214 lose conversation-aware routing with no fallback, and --use-dynamo-session-control / --use-legacy-dynamo-session-control / --dynamo-session-timeout-seconds now hard-error (Unknown option). Please gate the merge on #10214 being in the deployment baseline and add a release note for the removed flags. (Already acknowledged in the PR description — restating as the key pre-merge action.)
Suggested fix order
- Merge gate on #10214 + release note for removed flags.
- Harden converter output validation (inline).
- Decide skip-vs-abort policy for replay-less records (inline).
is_root is Falsestyle nit (inline).- Optional: integration coverage for fixed-schedule DAG replay and the
PAYLOAD_BYTES+ routing path (both currently unit-only; verified manually in this review).
Full living review + runtime receipts kept locally under artifacts/.
| root_trajectory_id: Optional root lineage to select instead of converting all roots. | ||
| """ | ||
| traces = _dynamo_traces_to_weka(input_file, root_trajectory_id) | ||
| if output.exists() and any(output.iterdir()): |
There was a problem hiding this comment.
Low — output validation ordering / non-directory error. The output check runs after the full conversion, so pointing --output at a non-empty dir wastes the whole conversion. More importantly, output.iterdir() raises a raw NotADirectoryError when --output is an existing file (confirmed at runtime: NotADirectoryError: [Errno 20] Not a directory).
Consider validating existence / empty / is-a-directory before calling _dynamo_traces_to_weka(...), and give the file case a clean message.
| return None | ||
| request = event.get("request") | ||
| replay = request.get("replay") if isinstance(request, dict) else None | ||
| if not isinstance(request, dict) or not isinstance(replay, dict): |
There was a problem hiding this comment.
Low–Medium — abort vs skip on replay-less records. A request_end with no agent_context.trajectory_id is silently skipped (returns None), but one that has a trajectory_id yet lacks a replay block aborts the entire conversion (confirmed: ValueError: Line N: agent request is missing replay metadata).
If real dynamo.request.trace.v1 captures can contain trajectory-tagged but non-replayable request_end events (e.g. tool/non-LLM requests), a single such record fails the whole file with no skip option — consider skipping them like the trajectory-less case. If the schema guarantees every trajectory-tagged request_end carries replay, this is fine as-is (matches the PR's 'reject incomplete' stance).
|
|
||
| # Validate and build schedule | ||
| for conv in self._conversation_source.dataset_metadata.conversations: | ||
| if conv.is_root is False: |
There was a problem hiding this comment.
Trivial nit. ConversationMetadata.is_root is a strict bool (default=True), so if not conv.is_root: is equivalent and reads more naturally than is False. The identity check would only matter for a bool | None field. Optional — current code is correct.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
could we finish this feature and add to infx repo? need this to update dynamo |
AIPerf now converts canonical
dynamo.request.trace.v1captures into replayable Weka traces and preserves session lineage on the wire. Existing deployments keep the deprecatednvext.session_controlpath by default, whileAIPERF_DYNAMO_SESSION_TRANSPORT=headersenables the new Dynamo session headers for migration testing.How This Was Implemented
aiperf synthesize dynamo-traceto preserve timing, token/cache shape, and direct parent-child topology.--use-dynamo-conv-aware-routingand--use-dynamo-session-controlas aliases for enabling session identity.nvextorheaderswithAIPERF_DYNAMO_SESSION_TRANSPORT;nvextremains the temporary default and emits a deprecation warning.Walkthrough
synthesize.py: convert request-trace JSONL into one Weka trace per root session.inference_client.py: emit eithernvext.session_controlorX-Dynamo-Session-ID/X-Dynamo-Parent-Session-ID.environment.py: expose the transport selector withnvextas the default.fixed_schedule.py: leave child dispatch to the branch orchestrator.Dependencies
nvextpath does not require the new Dynamo header support.Validation
git diff --checkpassed.