Subagent Conversation Support - #935
Conversation
7c3a2bf to
0e2df94
Compare
SkiHatDuckie
left a comment
There was a problem hiding this comment.
Some comments on dag.py. Many of these are questions that will also help me better understand what the DAG is doing and why.
There will be multiple batches of comment/requests as I go through this, but one of them will be dedicated to seeing how well this interface would work with trace replays like WEKA.
|
|
||
| :return: Sorted list of incomplete node IDs. | ||
| """ | ||
| return sorted(nid for nid in self._graph.nodes if nid not in self._completed) |
There was a problem hiding this comment.
What is the purpose of sorting the list in alphanumeric order?
There was a problem hiding this comment.
I removed the sorting. The ordering is now based on DAG node insertion order.
dbutenhof
left a comment
There was a problem hiding this comment.
I still haven't actually been able to finish this, but before I head out to hike I might as well submit my initial comments from Friday and this morning. 😁
SkiHatDuckie
left a comment
There was a problem hiding this comment.
Treat these as one suggestion. I can't find a good way of suggesting changes across different lines without making a large diff.
dbutenhof
left a comment
There was a problem hiding this comment.
Some additional comments / questions on the first (DAG) commit. (Whew.)
dbutenhof
left a comment
There was a problem hiding this comment.
OK, I started going through by commit, thinking it'd be easier, and then found out you removed the ENUMs and reorganized everything, so that exercise proved pointless. AH, well: I'll resume/restart tomorrow.
Generated-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Assisted-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
…ations Generated-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Assisted-by: Cursor AI Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Signed-off-by: Jared O'Connell <joconnel@redhat.com>
This makes the DAG function order based on the DAG node insertion order. Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
3fc3096 to
c3009b1
Compare
Generated-by: Cursor AI Signed-off-by: Jared O'Connell <joconnel@redhat.com>
sjmonson
left a comment
There was a problem hiding this comment.
Some initial notes on the worker code. Overall looks good, it was pretty easy to follow the code. Nits are a few inefficiencies and one bug.
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
| node_id=node_id, | ||
| agent_id=node.agent_id, | ||
| parent_node_ids=incoming_map[node_id], | ||
| turn_index=i, |
There was a problem hiding this comment.
turn_index should be the number of requests since initial, so branches increment turn index separately. Maybe it would be better to calculate this later from history length instead of here though.
| now = time.time() | ||
| for state in self.turns_queue: | ||
| nxt = state.next_node_ready_at() | ||
| if nxt is not None and nxt[1] <= now: | ||
| node_id, _ = nxt | ||
| state.claim_node(node_id) | ||
| return state, node_id | ||
|
|
||
| earliest: float | None = None | ||
| for state in self.turns_queue: | ||
| nxt = state.next_node_ready_at() | ||
| if nxt is not None and (earliest is None or nxt[1] < earliest): | ||
| earliest = nxt[1] |
There was a problem hiding this comment.
Maybe I am missing something but why do these need to be separate loops?
| now = time.time() | |
| for state in self.turns_queue: | |
| nxt = state.next_node_ready_at() | |
| if nxt is not None and nxt[1] <= now: | |
| node_id, _ = nxt | |
| state.claim_node(node_id) | |
| return state, node_id | |
| earliest: float | None = None | |
| for state in self.turns_queue: | |
| nxt = state.next_node_ready_at() | |
| if nxt is not None and (earliest is None or nxt[1] < earliest): | |
| earliest = nxt[1] | |
| now = time.time() | |
| earliest: float | None = None | |
| for state in self.turns_queue: | |
| nxt = state.next_node_ready_at() | |
| if nxt is not None: | |
| if nxt[1] <= now: | |
| node_id, _ = nxt | |
| state.claim_node(node_id) | |
| return state, node_id | |
| if earliest is None or nxt[1] < earliest: | |
| earliest = nxt[1] |
|
|
||
| raw = raw_values[0] | ||
| if isinstance(raw, str): | ||
| graph_data = ConversationGraphData.model_validate(json.loads(raw)) |
There was a problem hiding this comment.
| graph_data = ConversationGraphData.model_validate(json.loads(raw)) | |
| graph_data = ConversationGraphData.model_validate_json(raw) |
There was a problem hiding this comment.
Actually does this condition even make sense? Why would this ever be a string?
There was a problem hiding this comment.
According to the AI, yes it can be a string.
Yes, it is a string whenever the column comes from the synthetic branched dataset (or any HF/large_string JSON column). The condition exists because that payload is serialized for storage, not because the finalizer invents a string type.
I applied your suggestion locally.
| if turn.relative_timestamp is not None: | ||
| settings = settings.model_copy( | ||
| update={"relative_timestamp": turn.relative_timestamp} | ||
| ) | ||
| if turn.requeue_delay is not None: | ||
| settings = settings.model_copy( | ||
| update={"requeue_delay": turn.requeue_delay} | ||
| ) |
There was a problem hiding this comment.
I am very confused by what is going on here. Why does ConversationTurnData have a duplicate of every request setting field? And why is this code coping the model instead of just assigning the value?
There was a problem hiding this comment.
I'll look into this more tomorrow.
There was a problem hiding this comment.
I guess maybe the whole finalizer is a hack until #972 but I don't like how we have different code paths for column-wise vs single column datasets. Ideally at this point we are either all one or the other.
There was a problem hiding this comment.
This is a hard problem to solve without redoing everything. Because the existing pathways always assume it's flattened, but flattened data is very inefficient for multiturn conversations with subagents. But switching away from the existing flattened format as an option would break a lot of the advantages that the current column mapper provide.
So my thought is the dual code path format until we refactor the entire column mapper and preprocessor code sections.
| "For multiturn, one value is sampled per conversation and applied " | ||
| "to every turn (including branch turns that do not override it). " |
There was a problem hiding this comment.
This is a bug, it should be fixed not codified.
There was a problem hiding this comment.
Maybe fixed in a separate PR but please remove this and any other reference to "one value is sampled per conversation" that is not how this should work.
There was a problem hiding this comment.
I ended up fixing it, and while I was at it I switched to a way to separately specify the distribution for the first prompts, since in real world scenarios, you often include a big payload in the first message.
Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
dbutenhof
left a comment
There was a problem hiding this comment.
Quick skim through changes (all I've got brain left for right now); one minor new comment, and I believe I've resolved all my previous comments.
| ) | ||
| merge_after: int = Field( | ||
| description=( | ||
| "How many main-chain turns after at_turn the branch merges back. " |
There was a problem hiding this comment.
"main-chain"? You also used this (unhyphenated) for at_turn, but I think the "Main conversation" notation used in the docstring is more clear.
(Although "main conversation" still sounds a bit awkward when we're talking about a graph of conversations, I don't really have a better concise label ... 🤔)
There was a problem hiding this comment.
I switched it to main (parent) conversation.
This will allow us to better mock real world scenarios. Generated-by: Cursor AI Grok 4.5 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Signed-off-by: Jared O'Connell <joconnel@redhat.com>
Summary
This PR includes the initial implementation of the DAG, and basic synthetic data support to utilize that.
Details
Test Plan
Related Issues
TBD
Use of AI
git log
commit 26bad64
Author: Jared O'Connell joconnel@redhat.com
Date: Wed Jul 8 11:42:13 2026 -0400
commit 2599bc5
Author: Jared O'Connell joconnel@redhat.com
Date: Mon Jul 13 19:49:05 2026 -0400
commit 7980c20
Author: Jared O'Connell joconnel@redhat.com
Date: Wed Jul 15 17:50:15 2026 -0400
commit 6a81f9e
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 16 18:18:05 2026 -0400
commit d090274
Author: Jared O'Connell joconnel@redhat.com
Date: Fri Jul 17 00:30:57 2026 -0400
commit 345b2aa
Author: Jared O'Connell joconnel@redhat.com
Date: Fri Jul 17 01:03:47 2026 -0400
commit e45dd1f
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 23 19:23:55 2026 -0400
commit bdcab73
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 23 20:11:02 2026 -0400
commit ec3d0eb
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 23 20:23:53 2026 -0400
commit d59ee9b
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 23 22:35:47 2026 -0400
commit e98decc
Author: Jared O'Connell joconnel@redhat.com
Date: Thu Jul 23 22:41:46 2026 -0400
commit 49699dc
Author: Jared O'Connell joconnel@redhat.com
Date: Fri Jul 24 16:04:04 2026 -0400
commit f64f943
Author: Jared O'Connell joconnel@redhat.com
Date: Fri Jul 24 17:26:07 2026 -0400
commit c3009b1
Author: Jared O'Connell joconnel@redhat.com
Date: Fri Jul 24 17:59:43 2026 -0400
commit df949c9
Author: Jared O'Connell joconnel@redhat.com
Date: Tue Jul 28 14:15:20 2026 -0400
commit a32c57d
Author: Jared O'Connell joconnel@redhat.com
Date: Tue Jul 28 18:52:53 2026 -0400
commit d0dd8ef
Author: Jared O'Connell joconnel@redhat.com
Date: Wed Jul 29 15:29:58 2026 -0400
commit 425a45e
Author: Jared O'Connell joconnel@redhat.com
Date: Wed Jul 29 17:42:43 2026 -0400
commit 7943bb9
Author: Jared O'Connell joconnel@redhat.com
Date: Wed Jul 29 18:00:56 2026 -0400
Assisted-by: Cursor AI
Assisted-by: Cursor AI Claude Opus 4.6
Co-authored-by: Cursor cursoragent@cursor.com
Generated-by: Cursor AI
Generated-by: Cursor AI Claude Opus 4.6
Generated-by: Cursor AI Grok 4.5
Signed-off-by: Jared O'Connell joconnel@redhat.com