Agentic Profiling - #1029
Draft
Sajeeth-Wimalasuriyan wants to merge 26 commits into
Draft
Conversation
Adds an OpenAI-compatible chat assistant under src/view/src/agenticprofiling/ that can drive Optiq's trace and compute views through named tools, gated behind a settings flag with a user-supplied endpoint and configurable route. Vendors cpp-httplib for the HTTP client. Co-authored-by: Cursor <cursoragent@cursor.com>
… tools The assistant sent its API key somewhere it should not have and trusted numbers the model made up. Both are fixed here, and the tool layer is broken up so the parts handling untrusted input can be reviewed on their own. Credentials: - Stop following redirects. cpp-httplib drops only a fixed set of headers on a cross-host redirect and the Azure subscription key is not one of them, so following one could hand the key to another host. A 3xx now reports why. - Refuse to send the key over plain http unless the endpoint is loopback, so a local model server still works but a remote one cannot take it in clear. - Elide query strings from logs and error text, in case a key was pasted in. - Stop reading the pre-provider credential entry as a live fallback. Any endpoint without a key of its own inherited it, so a second endpoint at a different vendor would have been sent the first one's key. It is now a one-shot migration at load. Reset also clears the keys of endpoints it discards, instead of orphaning them in the credential store. Model-supplied input: - Reject non-finite time ranges. "end <= start" is false for NaN, so every range guard in the feature let NaN and infinity through. - ShowPanel no longer reports success when there is no widget to change; it was moving global settings behind the user's back on compute traces. - Clamp the SQL offset, bound JSON array arguments, escape LIKE wildcards so a name containing % or _ is matched literally, and reject malformed filters instead of quietly running a broader query than was asked for. - goto could highlight nothing and still report success; it now navigates to an event that took and says so when none did. get_summary no longer formats an empty summary as though it were a measurement. - Bookmark slots are range-checked on every entry point, not only on save. - Tab lookup now needs a unique, long-enough match; a one-letter guess used to activate whichever tab happened to come first. Structure: - Split SQL fragment building into rocprofvis_ai_tool_query and the model-facing schema into rocprofvis_ai_tool_schema. The query module is the one place a bad argument can become bad SQL; the schema module touches no view state, which is what makes it safe on the HTTP worker thread. - Replace the 1200-line StartAssistantTool if-chain with one function per tool behind a dispatch table. Bodies were moved verbatim. - Fold four copies of a lowercase helper into to_lower_copy, and two copies of tab-name matching into TabContainer::FindTabByLabel. Also fixes the main view losing ImGui state - column widths, tree expansion, scroll position - every time the assistant panel was toggled, because it only entered a child window when the panel was open. Builds verified with ROCPROFVIS_ENABLE_AGENTIC_PROFILING both on and off. Co-authored-by: Cursor <cursoragent@cursor.com>
…nel visibility Two problems on the turn loop and one on panel toggling. Warmup: - TryStartSummaryWarmup parks a fetch that has no tool call behind it. If the trace closed or the user switched tabs while it was in flight, FinishCurrentTool saw an empty tool queue, read that as "tools are done", and fell through to ContinueAfterTools - starting an HTTP round without ever appending the queued user message. The model was asked to answer a question it had not been given. Guarded in FinishCurrentTool and again in PollToolFetch, so neither path can reach the dispatcher during a warmup. - The queued question now only goes out on the trace it was asked about. A tab change during the preload resets the turn and says so instead of answering about the wrong trace. Fetch contention: - Drop ASSISTANT_MAX_FETCH_RETRIES. A tool that piggybacked on someone else's in-flight request used to get two retries and then format whatever rows had landed, which could be another query's result. Retries are now bounded by the original 45s wait deadline instead of a count, and a tool that never gets to issue its own query reports a timeout rather than borrowing rows. BeginFetchWait keeps the first wait's start time across retries so the deadline does not reset each time. Panel visibility: - Add AppWindow::ApplyPanelVisibilitySettings and route both the View menu and OptiqActions::ShowPanel through it, so the two cannot drift. It applies the toolbar, details, topology, and histogram flags to every live layout, and runs once at Init so a restored setting is reflected on the first frame. - Setting-backed panels no longer require the active tab to be a TraceView. They follow the View menu and update every open system trace, so asking to hide the topology panel with a compute tab in front now works instead of reporting failure. Only the trace-local panels (minimap, annotations) still need an active TraceView. Co-authored-by: Cursor <cursoragent@cursor.com>
rocprofvis_ai_tools.cpp had grown to 2747 lines doing three jobs at once: the dispatcher, the tools that drive the UI, and the tools that read the trace. The two halves carry different hazards - a UI tool goes through OptiqActions and answers in the same call, while a data tool has to reason about request ids and result slots it shares with the rest of the app - so they are now separate files that can be read and reviewed on their own. - rocprofvis_ai_ui_tools.cpp: goto, show_panel, switch_tab, flow_arrows, annotate, bookmark, measure, reset_view, offer_next_steps. This is now the only file in the executor that names OptiqActions, so the read half cannot mutate the UI even by accident - a property that was previously only convention. - rocprofvis_ai_data_tools.cpp: the eleven query tools, every formatter they use, the trace briefing and activity-chart helpers, and FinishAssistantFetch. - rocprofvis_ai_tools.cpp keeps StartAssistantTool and the five helpers both halves need. Two of those are worth naming, because they are why goto and annotate are not purely UI tools: SelectedOrFullTimeRange, since annotate stores the window the user was looking at, and FindComputeKernel, since goto selects a kernel by the same loose name kernel_metrics accepts. - rocprofvis_ai_tools_internal.h is private wiring. Each body file owns its own handler table and hands it over through an accessor, so the handlers keep internal linkage and adding a tool never touches the dispatcher: a schema entry, a body, and a line in that file's own table. Bodies were moved verbatim - no behaviour change. Also drops four includes the executor no longer used (spdlog, event manager, events, iomanip), and brings the .agents/UI.md inventory back in step: it still described the module as "four files" and had never been updated for the earlier tool_query and tool_schema split. The UI.md hunks documenting ApplyPanelVisibilitySettings and the shared-request-id rule belong with the previous commit and rode along with this one. Not build-verified: ROCPROFVIS_ENABLE_AGENTIC_PROFILING is off by default and this machine has no mbedtls submodule checked out. Verified instead by checking that all twenty tool names still resolve across the two tables, that every moved symbol is defined exactly once, and that neither body file references the other's helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
… in-app analysis.
…ripts can read traces without clobbering the UI tables. Scripts get optiq.trace, selection, table().fetch(), and Track.events() on a dedicated interpreter thread, with Future progress forwarding and Catch2 coverage against the sample RPD. Debug Windows builds copy python3xx.dll beside the exe instead of the import lib.
…hows result text in the view. The editor is a View-menu singleton on the same request/poll path as other controller work, with run/cancel, load/save, and selection as script context. PYTHON.md documents the in-app API.
…o scripts can do nesting-aware and counter analysis. The controller already tracked all three, but copy_event dropped them, which put self time, flame-chart depth and any counter reading out of reach from a script. value is None on interval events rather than 0.0, so mixing event and sample tracks fails loudly instead of skewing toward zero. PYTHON.md gains idle-gap, flame-depth, counter and hot-name examples and corrects two claims it had wrong: system table cells always arrive as strings, and the even-spacing example needs a level filter on nested tracks. Catch2 covers the new fields against the sample RPD.
Keep model-written code behind explicit user approval and present scripts in a compact per-trace workspace with progress and results. Co-authored-by: Cursor <cursoragent@cursor.com>
Sajeeth-Wimalasuriyan
requested review from
a team,
amokiche-amd,
drchen-amd,
tomk-amd and
vstempen
as code owners
August 27, 2026 03:18
Sajeeth-Wimalasuriyan
marked this pull request as draft
August 27, 2026 03:19
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+171
to
+190
| switch(property) | ||
| { | ||
| case kRPVControllerFutureProgressPercentage: | ||
| { | ||
| std::lock_guard lock(m_mutex); | ||
| uint64_t clamped = value; | ||
| if(clamped > UINT16_MAX) | ||
| { | ||
| clamped = UINT16_MAX; | ||
| } | ||
| m_progress_percentage = static_cast<uint16_t>(clamped); | ||
| result = kRocProfVisResultSuccess; | ||
| break; | ||
| } | ||
| default: | ||
| { | ||
| result = UnhandledProperty(property); | ||
| break; | ||
| } | ||
| } |
Comment on lines
+199
to
+213
| switch(property) | ||
| { | ||
| case kRPVControllerFutureProgressMessage: | ||
| { | ||
| std::lock_guard lock(m_mutex); | ||
| m_progress_message = value ? value : ""; | ||
| result = kRocProfVisResultSuccess; | ||
| break; | ||
| } | ||
| default: | ||
| { | ||
| result = UnhandledProperty(property); | ||
| break; | ||
| } | ||
| } |
… the script contract Isolation. The assistant and scripting code no longer sits inside shared core files. The Settings dialog's Assistant page and its credential-store access, the TraceView methods that exist only for OptiqActions, and the DataProvider script path all moved into conditionally compiled files, so a feature-off build does not carry them. Reading and writing the endpoint list deliberately stays in rocprofvis_settings_manager.cpp: it runs unguarded, so moving it would erase a saved configuration in a build with the assistant off. Bugs found and fixed: - The Python runtime reported success from a later Init() when startup had already failed, parking every subsequent script on a future that could never complete. - JSON numbers from the model were cast straight to uint64_t, which is undefined for NaN, infinity, or anything past the type's range. - ScriptEditor ignored a failed CancelScript() and sat on Running forever. - ComputeView installed a this-capturing progress callback with an empty destructor; TraceView cleared six callbacks but not that one. - SelectAnalysisTab wrote the global show_details_panel but applied it to one trace, desyncing every other open tab. - PyThreadState_SetAsyncExc ignored a >1 return, which CPython requires be reverted. - annotate mixed a model-supplied timestamp with a live-read view range and reported unconditional success, so a stale anchor was described as the selected interval. It now validates against the trace and says where the note actually landed. Duplication and dead code. View::to_lower_copy, TrimCopy and ToLowerAscii all duplicated existing Core::String helpers. Removed the PanelBacking enum whose only consumer asked one question of it, an unreachable warmup branch, an unused result code, and a completion event posted to a listener that tab-close had already destroyed. Script contract. The model's only API reference is the tool schema, while the real documentation lives in PYTHON.md, which nothing loads - so it guessed argument types and got them wrong. The schema now states that tracks= takes Track objects rather than ids, that where/group are strings, that row cells are often str, that categories are trace-defined, and that the same GPU work appears on several tracks so naive concatenation double-counts. The matching binding errors say how to correct them. Also: the Script tab's source/result split is now draggable, using the same splitter as the Ask Optiq dock. Co-authored-by: Cursor <cursoragent@cursor.com>
The tool schema was the model's only account of the optiq API, and it was wrong. It listed ten arguments to Table.fetch where the binding takes twelve - filter and group_columns sit in the middle, so any positional call shifted everything after where - and it gave no type for sort_column, which is a zero-based column index rather than a name. Scripts failed on that mismatch and the model, having no correct reference to consult, guessed variants until it gave up. PYTHON.md had the signature right all along; nothing loads it. The description now carries the full argument list in order with types, says to call fetch with keyword arguments, and warns that track and event names are free-form strings that need not match the UI label or list_tracks output - the guess behind an IndexError on tracks[0]. The script prompt adds that two failures on the same call mean the signature is wrong rather than the line, so re-read the argument list instead of trying another variant. Also connects offer_next_steps to scripting. It was already producing script-shaped follow-ups without knowing it: the best next steps are the questions an answer raised but could not settle, and those are worth offering precisely when answering them needs a script. The script prompt now says to volunteer one when the aggregates leave a real question open - is the spacing regular, what is the distribution behind that mean, how much of the window is concurrent - and to say what the number would tell the user rather than naming a statistic. Offering stays free; running still requires the user. Co-authored-by: Cursor <cursoragent@cursor.com>
Two findings that both came down to a boundary being assumed rather than enforced. Ask Optiq issued its table reads on the request ids and model slots the Event, Sample, Search and Top Events tabs render from, so asking a question rewrote the rows, sort and row count the user was looking at. It now reads through its own request ids, its own controller tables and its own model slots, routed by a client id on the request params. Splitting only the ids would have been worse than leaving it alone - two fetches would then race one shared SystemTable row cache - which is why search_table_alloc and analysis_events_table_alloc exist. The Python guardrail was described as containment it does not provide. An allowlisted module is a real module object, so json.dumps.__globals__ leads back to the unrestricted builtins and ().__class__ leads to object.__subclasses__, neither of which needs a blocked builtin or an import. Scripts are now screened by parse tree for interpreter-internal names before they run, and SCRIPTING.md and PYTHON.md say plainly that approval before a run is the boundary and this is defence behind it. Also from the review: cancel targets the session it names instead of whichever script is executing; ensure_types is all-or-nothing, so a partial failure is retried rather than remembered as success; trace-close waits with a bound and abandons handles rather than freeing them under a script still writing through them; endpoint names are de-duplicated because the name is the credential-store key; history is trimmed and the briefing is sent once per trace rather than once per turn; tool calls per round are capped; model-supplied ids and filter shapes are refused rather than half-read; sort columns resolve by name. import optiq now works as documented, and the stale comments about tab-close posting a script completion event are gone - that post was removed in 6913f4b. Co-authored-by: Cursor <cursoragent@cursor.com>
cpp-httplib was the one dependency copied into the tree rather than referenced, which put a 21,842-line header into every diff and left its version recorded only in a CMake comment. It is now a submodule pinned to v0.53.1, matching how the other twelve thirdparty dependencies are tracked. The checked-out header is byte-identical to the copy it replaces, and upstream keeps httplib.h at its repo root, so the include path and the cpp-httplib INTERFACE target are unchanged. Configure now fails with an explicit message when the submodule is missing, since the alternative was a confusing missing-include error deep in the build. CI already checks out submodules, so no workflow changes were needed. Co-authored-by: Cursor <cursoragent@cursor.com>
A non-UI client issues its fetch through its own private table so its rows cannot overwrite what a tab is showing, but the completion path still resolved the shared table to read the column list from. Nothing had run this query on that one, so it reported zero columns and every returned row decoded to empty cells. The rows were always arriving. The assistant was told it had received a page of blanks, and reported to the user that the trace held no event rows or uuids - which was true of what reached it, and wrong about the trace. Co-authored-by: Cursor <cursoragent@cursor.com>
… return kernel_instances matched kernel names exactly, so a search for a mangled C++ signature had to reproduce it character for character or match nothing - the schema had promised 'exact or unique name' all along. It also sorted by column 0 ascending, which picked a row by id rather than by duration while the model described it as the slowest, and it inherited the timeline selection, so a goto to an interesting window silently hid every instance outside it. top_events is an aggregate - name, Invocations, DurationTotal - and carries no uuid, but the prompt and schema both implied its rows could be handed to goto. Now they say which tools return an event and which only return a name. Sort fallbacks ask for their column by name before falling back to an index, so a reordered table sorts by the same thing rather than by whatever moved into that slot. Co-authored-by: Cursor <cursoragent@cursor.com>
goto both clicked and highlighted the first event. Where the two overlap the highlight border wins, so the event it had selected drew exactly like the eleven around it and the user had nothing to look at. The focal event is now clicked and left unhighlighted, so it renders as a real selection with the rest as context. A missing range no longer fails the whole call either. Only the framing needs a window, so goto derives one from the event when the model names an event without a range, and still selects when it cannot. Co-authored-by: Cursor <cursoragent@cursor.com>
IsPanelVisible, AreFlowArrowsVisible, Notify, ClearRange, RevealTrackInTopology, ShiftClickEvent, ClearEventSelection and SelectWorkload have no callers: they read as a capability catalogue written ahead of tools that were never added. Deleting them orphaned three TraceView getters that existed only to back them, so those go too. No behaviour change - the model could not reach any of this, because none of it was wired to a tool. Co-authored-by: Cursor <cursoragent@cursor.com>
…running rocprofvis_ai_assistant.cpp carried three jobs at 1783 lines. The standing instructions are content rather than code and now live in rocprofvis_ai_prompts.cpp, where they can be read and revised as prose; the ImGui half moves to rocprofvis_ai_assistant_render.cpp, which touches no conversation state. What remains is the turn machine, at 945. A tool called while a trace was still opening returned 'still loading' in microseconds, and nothing made the model wait - so on a large file every tool in every round failed instantly and the whole round budget burned before the trace was readable. That wait now parks and polls like any other fetch. The fetch deadline moves to 350s with the elapsed time shown in the status, because cutting a live query off and reporting a timeout is worse than making the user wait: the model answers around the gap instead of reporting it. TrimConversation could not bound one investigation. It only cut on a user message, and a dive is a single question followed by many rounds of tool replies, so it never fired while the transcript grew past anything the endpoint would accept. Old tool replies are now compacted in place, which keeps each tool_call_id paired with the call that asked for it. Tool arguments and results are logged at debug, so an answer can be checked against what the model was actually given. That is what found the table decoding bug. Co-authored-by: Cursor <cursoragent@cursor.com>
Naming one event as event_uuid and again inside events[] is the natural way to write 'this is the one I mean, and here is the set', so the same uuid arrives twice. The second copy highlighted the event the first had selected, the highlight border wins where the two overlap, and the focal point vanished back into the group it was meant to stand out from. Targets are deduplicated by uuid before anything is marked. Co-authored-by: Cursor <cursoragent@cursor.com>
The sort column is resolved by name, but that name is looked up in a header read off a table nothing has fetched into yet - so the first query of a session has no header to search and falls through to an index. That index was 2, which is Duration only on the top-events table; on the kernel-instance table column 2 is category, and every row shares one, so descending order was arbitrary. The effect was a silent wrong answer: asked for the slowest instance of a kernel whose aggregate maximum is 427,747 ns, it returned a 2,927 ns row and described it as the slowest. The model caught the contradiction itself by comparing against kernel_metrics. Fallback indices are now per table and derived from the real headers. Time columns are documented as nanoseconds in the schema, because the same answer quoted 427,747 ns as milliseconds and pinned that figure in a timeline annotation. Co-authored-by: Cursor <cursoragent@cursor.com>
JsonU64FromDouble only rejected values that would overflow uint64_t, but a double stops representing integers exactly at 2^53. An event uuid packs an id, a node and an operation into 64 bits and lands near 2^61, where the gap between representable doubles is 512, so a uuid the model sent as a JSON number came back rounded to the nearest multiple of 512 and was used without complaint. goto selected an event the timeline had never heard of, and event_details reported another kernel's arguments and flow links - both answered confidently, with nothing downstream able to notice. Ids now travel as strings, which JsonU64 already parses exactly: the three event_uuid parameters are typed string, and __uuid is printed quoted in result rows so the model copies it back the same way. The narrowing guard drops to 2^53 as a backstop, turning a number that cannot be held exactly into a tool error the model can see and correct rather than a silent wrong answer. Timestamps are unaffected - they read through GetDouble, not this path - and every other JsonU64 caller is a track id, kernel id or clamped offset, all far below the limit. Co-authored-by: Cursor <cursoragent@cursor.com>
The kernel instance fallback pointed at 15, read off a rendered result row. But FormatTableSnapshot hides the __ service columns on the way out, and the table carries __op at position 0, so every visible position is one short of the index the controller sorts by. 15 is end, not duration. Sorting by end descending returns the last dispatch of the trace rather than the longest one, and the controller reports nothing amiss - it sorts whatever sits at the index it was given. The slowest matvec came back as a 116 us instance that happened to run last, while the real maximum was 12.77 ms, and the tool stated it as fact. Only reachable on the first query of a session, since after that the column resolves by name against the full header, which is why the same call answered correctly the second time. Corrected to 16, with the layout in the comment now showing __op so the next reader counts the same columns the ABI does. The top events fallback of 2 is right as it stands: that table has no __op. Co-authored-by: Cursor <cursoragent@cursor.com>
FetchEvent only wrote basic_info.id when it found the event by scanning the track data already in memory. Reached by uuid from a table row, with that track's chunk not loaded, the scan missed and the id kept the zero it was constructed with - while the extended data fetch that follows went on to fill in the name, timestamps and track. So the event came back looking complete and correct in every field except the one identifying it. That zero then travelled. The flow list copies basic_info.id into the entry standing for the event itself, so event_details reported a uuid of 0 for the very event it had just been asked about, and nothing could navigate to the far end of a flow link. Call stack frames are synthesised against it as owner. The events view prints it as the event id. The timeline arrows were the only consumer that kept working, because they position from track and timestamps and never read the id at all, which is why this stayed invisible in the UI. The caller already knows which event it asked for, so record it up front and let the scan fill in the rest. Nothing compares this field against a sentinel, so populating it cannot break a lookup that expects zero to mean absent. Co-authored-by: Cursor <cursoragent@cursor.com>
The drop pass cuts whole exchanges and lands only on a user message, which is the right boundary: everything after one belongs to a single topic. But a single investigation contains exactly two user messages - the question that started it, and the nudge BeginFinalAnswer appends telling the model to stop calling tools and write up what it gathered. That nudge is appended in the user role, because that is the role the model takes instruction from, and the trimmer could not tell it apart from a fresh question. So on the one round that matters it cut to the nudge. A 65-call investigation went into its final round with 76 messages and came out with 2: the system prompt and an instruction to write up numbers that had just been deleted along with the question that asked for them. The model refused to invent them and said so, which was the only correct answer available to it. The failure scaled the wrong way. Short investigations stay under the message cap and never trim, so this only appeared once the assistant had done enough work to be worth reading - the more thorough the run, the more certain it was to lose all of it. Cuts now stop at the start of the live turn, found by taking the last user message, or the second to last on the final round where the nudge occupies that slot. Earlier exchanges are still droppable, since those are cheap to lose, and the character budget still compacts old tool replies in place. Only the evidence the answer is being written from is now off limits. Co-authored-by: Cursor <cursoragent@cursor.com>
Compute support reached into all ten assistant files, but of the twenty tools only three did anything on a compute trace: get_summary and kernel_metrics returned the same top-kernel list, and goto could select a workload. The other seventeen refused, each through its own is_compute branch. Everything the assistant is actually good at - finding an outlier instance, drilling into an event, following flow arrows, correlating tracks - needs a timeline, and a compute trace has none to hang it on. So the branches are gone and the refusal happens once, in StartAssistantTool, beside the check for no trace being open. The panel is a singleton shared across tabs, so the guard is load-bearing rather than cosmetic: the user can open it on a system trace and then bring a compute tab to the front. ComputeView no longer offers the toolbar button. That takes four paragraphs of compute rules out of the system prompt, where they were spent on every request including the system traces that could never use them, and takes the compute selection plumbing out of AssistantToolContext and the compute headers out of five translation units. Verified against sample/rocpd-transpose.db afterwards: all twenty tools fire, no errors, no criticals. The three fixes from earlier today still hold - the slowest kernel instance comes back as 8067173 ns on the first cold call and cross-checks against kernel_metrics, uuids arrive as exact strings, and flow entries carry real uuids. kernel_metrics in particular is unharmed, which mattered because it was the only tool with both a compute and a system branch. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not review this is a draft to test agentic profiling and python interpretation