feat(predict): add ReActV2 forced submit - #58
Conversation
Greptile SummaryThis PR adds a two-tier forced-submit recovery to
Confidence Score: 3/5Safe to merge once the history double-mutation in tier 1 is resolved; all other findings are non-blocking quality issues. The tier 1 path combines dspy/predict/reactv2.py — specifically the try-except block around lines 169-173 in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[forward loop exits\nno submit returned] --> B[_forced_submit]
B --> C[force_tool_call_config\nadapter]
C --> D{config non-empty?}
D -- yes --> E[self.react with forced\nsubmit tool choice]
D -- no --> E
E --> F{pred has submit\ntool call?}
F -- no / error --> I
F -- yes --> G[execute submit tool]
G --> H{result valid?}
H -- ValueError --> I
H -- ok --> J[_append_tool_turn\nhistory]
J --> K[history.append_output\nresult]
K --> L{dspy.Prediction\n** result}
L -- TypeError --> I
L -- ok --> M[return termination_reason=\nforced_submit]
I[Tier 2: render history\nas text] --> N[self.extract\ntrajectory=text]
N --> O{any non-None\noutput fields?}
O -- yes --> P[history.append_output\nextract result]
P --> Q[return termination_reason=\nextract]
O -- no / error --> R[return termination_reason=\nbreak_reason or failed]
Reviews (1): Last reviewed commit: "feat(predict): add ReActV2 forced submit" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| history.append_output(result) | ||
| return dspy.Prediction(history=history, termination_reason="forced_submit", **result) | ||
| except TypeError as err: | ||
| logger.debug(f"Forced submit result was not a valid output mapping: {_fmt_exc(err)}") |
There was a problem hiding this comment.
dspy.Prediction
history.append_output(result) and dspy.Prediction(history=history, termination_reason="forced_submit", **result) are batched in the same try block. If append_output succeeds but dspy.Prediction(**result) then raises TypeError (e.g., the signature has an output field named history or termination_reason, causing a duplicate-keyword error), history already has a committed complete output frame. Control then falls through to tier 2, which calls history.append_output(result) again — leaving history with two complete=True output frames for the same episode. This silently corrupts the history that gets returned to the caller.
The fix is to scope the except TypeError only around dspy.Prediction(...), leaving history.append_output(result) outside the try-except, so a TypeError from the constructor never triggers tier 2 on an already-closed episode.
| adapter = dspy.settings.adapter or dspy.ChatAdapter() | ||
| call_config = adapter.force_tool_call_config("submit") |
There was a problem hiding this comment.
When adapter.use_native_function_calling is False, force_tool_call_config returns {}, so call_config = {}. self.react(..., config={}, ...) is then functionally identical to a plain self.react(...) call — exactly the call that just failed (or returned empty tool calls) in the main loop. For these adapters, tier 1 will almost certainly produce the same broken response and burn a second LM call before falling through to the extract tier. Consider gating tier 1 behind a check that call_config is non-empty.
| for event in history.frames: | ||
| if isinstance(event, dict): | ||
| for key, value in event.items(): | ||
| lines.append(f"[Input] {key}: {value}") | ||
| continue |
There was a problem hiding this comment.
[Input]
The guard if isinstance(event, dict): ... continue labels every key in a raw dict frame as [Input]. However, HistoryEntry = HistoryFrame | dict[str, Any] allows dict entries that contain output-field keys (e.g., dict entries passed directly in a History(frames=[{...}]) constructor). These would be misrepresented as inputs in the trajectory text fed to the extract LM, which could confuse the model about what has already been answered.
| def test_forced_submit_runs_when_loop_returns_no_tool_calls(): | ||
| lm = DummyLM( | ||
| [ | ||
| {"next_thought": "No call.", "tool_calls": []}, | ||
| {"next_thought": "Force submit.", "tool_calls": [{"name": "submit", "args": {"answer": "done"}}]}, | ||
| ] | ||
| ) | ||
| dspy.configure(lm=lm) | ||
| react = ReActV2("question -> answer", tools=[add]) | ||
|
|
||
| result = react(question="test") | ||
|
|
||
| assert result.answer == "done" | ||
| assert result.termination_reason == "forced_submit" |
There was a problem hiding this comment.
The new test only exercises the happy path of tier 1 (no_tool_calls → forced react → forced_submit). The tier 2 extract path (triggered when tier 1 also fails), and the scenarios where the loop breaks due to parse_error or context_overflow, are entirely untested. A test where force_tool_call_config returns {} (or the LM still returns no submit tool call) would exercise the self.extract(trajectory=...) path and verify the termination_reason == "extract" return.
1 participant
Summary
Stack
Validation
uv run --extra dev pytest -q tests/predict/test_reactv2.py