Transfer V1 completion logprobs without copying - #1804
Conversation
ApprovabilityVerdict: Approved Minor performance optimization removing an unnecessary list copy. The change is well-documented and doesn't alter runtime behavior beyond avoiding allocation overhead. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2daa0c3e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mask=[False] * len(gen_prompt) + [True] * len(comp_ids), | ||
| logprobs=list(tokens.completion_logprobs) if tokens else [], | ||
| # TurnTokens is discarded after commit, so transfer its logprobs without copying. | ||
| logprobs=tokens.completion_logprobs if tokens else [], |
There was a problem hiding this comment.
Detach logprobs before committing the trace
When a caller keeps or reuses the Response/TurnTokens object after commit(), this stores the same mutable completion_logprobs list on the long-lived trace. Any later normalization, clearing, or buffer reuse by a custom train client mutates trace.nodes[-1].logprobs, and Branch.logprobs/trace dumps will then report corrupted training logprobs; the previous list(...) copy isolated committed graph state from the transient response object.
Useful? React with 👍 / 👎.
Overview
Transfer the transient
TurnTokens.completion_logprobslist directly into the assistantMessageNodeduring graph commit instead of materializing a second list.Reasoning
_commit_turnconstructs the node withMessageNode.model_construct, so it can preserve the already-typed list's identity without another validation or copy.TurnTokensis a per-response carrier that is discarded after commit, while graph and training consumers treat node logprobs as read-only. The graph node can therefore become the long-lived owner without changing logprob values, ordering, serialization, branching, or partial-logprob behavior.Performance
A PEP 723 microbenchmark measured 2,000,000 logprob values over nine repetitions using
time.perf_counterandtracemalloc:This removes 2,000,000 copied references (about 16,000,000 bytes of pointer storage) and the transient duplicate list buffer. Long-lived trace storage remains unchanged at one logprob list.
Note
Low Risk
Single-line ownership change in graph commit with no intended change to logprob values or training semantics; only risk is if something mutates the list before discard (unchanged from prior copy behavior for readers).
Overview
When committing an assistant turn in
_commit_turn, the graph node now takes ownership ofTurnTokens.completion_logprobsinstead of building a second list withlist(...).TurnTokensis only used for that commit path and is dropped afterward, so the assistantMessageNodebecomes the long-lived holder of the same list. This matches the existingmodel_constructpattern for large token slices and avoids extra allocation on long completions.Reviewed by Cursor Bugbot for commit 2daa0c3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Transfer V1 completion logprobs without copying in
_commit_turnIn
graph.py,_commit_turnnow assignstokens.completion_logprobsdirectly toMessageNode.logprobsinstead of wrapping it inlist(). SinceTurnTokensis discarded after commit, the copy was unnecessary.Macroscope summarized 2daa0c3.