refactor(models): move turns off the record wire; add root_correlation_id#1118
refactor(models): move turns off the record wire; add root_correlation_id#1118ajcasagrande wants to merge 1 commit into
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@95fde25f60519739b1c90034143da371aa3ebedaRecommended 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@95fde25f60519739b1c90034143da371aa3ebedaLast updated for commit: |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
afebd87 to
95fde25
Compare
WalkthroughThis PR adds a ChangesRoot correlation id and payload field relocation
Estimated code review effort: 2 (Simple) | ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/workers/test_inference_client_enrich_record.py (1)
31-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
root_correlation_idcoverage to the DAG-fields propagation test.
test_dag_fields_propagateverifiesagent_depthandparent_correlation_idsurvive the_enrich_request_recorddowncast, butroot_correlation_id— added toRecordContextin this same cohort — isn't asserted here. Since correct propagation of this field depends on it staying in sync withRecordContext.model_fieldsin the downcast filter, a regression (e.g., accidental exclusion) wouldn't be caught by existing tests.✅ Suggested test extension
def test_dag_fields_propagate(self): - ri = _make_request_info(agent_depth=3, parent_correlation_id="p") + ri = _make_request_info( + agent_depth=3, parent_correlation_id="p", root_correlation_id="root-1" + ) record = RequestRecord() enriched = InferenceClient._enrich_request_record(record, ri) assert enriched.request_info.agent_depth == 3 assert enriched.request_info.parent_correlation_id == "p" + assert enriched.request_info.root_correlation_id == "root-1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/workers/test_inference_client_enrich_record.py` around lines 31 - 46, Update the existing `TestEnrichRequestRecord.test_dag_fields_propagate` coverage to also assert that `root_correlation_id` survives `InferenceClient._enrich_request_record` when `RequestRecord.request_info` is downcast to `RecordContext`. Use the same `_make_request_info(...)` setup and add an assertion on `enriched.request_info.root_correlation_id` alongside the existing `agent_depth` and `parent_correlation_id` checks so the field stays aligned with `RecordContext.model_fields`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/workers/test_inference_client_enrich_record.py`:
- Around line 31-46: Update the existing
`TestEnrichRequestRecord.test_dag_fields_propagate` coverage to also assert that
`root_correlation_id` survives `InferenceClient._enrich_request_record` when
`RequestRecord.request_info` is downcast to `RecordContext`. Use the same
`_make_request_info(...)` setup and add an assertion on
`enriched.request_info.root_correlation_id` alongside the existing `agent_depth`
and `parent_correlation_id` checks so the field stays aligned with
`RecordContext.model_fields`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8597d6b4-3315-4395-919d-0893b0f77479
📒 Files selected for processing (5)
src/aiperf/common/models/record_models.pysrc/aiperf/records/record_processor_service.pysrc/aiperf/workers/inference_client.pytests/unit/common/models/test_record_context.pytests/unit/workers/test_inference_client_enrich_record.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| cancellation_time_ns=cancellation_time_ns, | ||
| agent_depth=record.request_info.agent_depth, | ||
| parent_correlation_id=record.request_info.parent_correlation_id, | ||
| root_correlation_id=record.request_info.root_correlation_id, |
There was a problem hiding this comment.
root_correlation_id is copied into MetricRecordMetadata here, but no upstream SampledSession/TurnToSend/Credit/Worker._create_request_info path ever sets RequestInfo.root_correlation_id, so exports will always contain None instead of the depth-0 session id. Fix: carry root_correlation_id through credit issuance and request creation, defaulting root sessions to their own x_correlation_id and preserving the ancestor root for DAG descendants.
🤖 AI Fix
Update src/aiperf/timing/conversation_source.py SampledSession and child builders, src/aiperf/credit/structs.py TurnToSend/Credit/TurnToSend.from_previous_credit, src/aiperf/credit/issuer.py credit construction and join dispatch, src/aiperf/timing/_branch_orchestrator_state.py PendingBranchJoin, src/aiperf/timing/branch_orchestrator.py join state creation, src/aiperf/timing/_branch_orchestrator_spawn.py child creation, and src/aiperf/workers/worker.py Worker._create_request_info so root_correlation_id is populated on every RequestInfo.
Stacked on #1102 (
ajc/metrics-accumulator-engine) — retargets tomainwhen it merges. Part of the agentx carve-out series (#1115, #1116, #1117).What
turns/system_message/user_context_messagefromRecordContexttoRequestInfoand deletesRequestRecord.turns, so the Turn list physically cannot cross the ZMQ hop. This completes the payload-bytes counting story feat(metrics): accumulator engine with records-pipeline wiring #1102 started: input/media counts are derived from canonical wire bytes (payload_bytes+extract_payload_inputs), not from turns carried over the bus. On the base tree,RequestRecord.turnsis populated, nulled in the record processor, and never read (grep-verified) — this PR removes that dead per-request allocation (copy_with_stripped_media) ininference_client.py.root_correlation_idtoMetricRecordMetadataandRecordContext, completing the existingagent_depth/parent_correlation_idDAG-correlation family. Consumed by the swim-lane viewer (feat(analysis): swim-lane viewer and aiperf analyze CLI #1116) for lane grouping (which has a legacy parent-walk fallback until the producer lands with the agentic core).Deliberate exclusions (stay with the agentic core in #1070)
source_trace_id/outer_idx/inner_idx/kind,context_overflow_*,cache_bust_marker/cache_bust_target,ReplayTurnReference— every consumer lives in weka/credit/scenario code not yet on main; shipping them here would be dead schema.context_overflow_skipis also excluded despite a graceful.get()read in #1116: its documented semantics reference RecordProcessor/RecordsManager machinery that only exists in #1070.Evidence
pytest -n auto tests/unit/common/models/ tests/unit/records/— 765 passed, 2 skippedpytest --collect-only tests/unit/clean;import aiperf+ field-placement asserts passField(description=...)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes