Repository navigation
Add field_markers opt-out to ChatAdapter - #10324
ramyasreenagarajan wants to merge 12 commits into
Conversation
[Low impact] The change affects only callers who opt out of markers in ChatAdapter. Not safe to merge: package-loading errors remain, and marker-free answers do not produce field-stream events. Findings
SummaryAdds
Reviews (12) · Last reviewed commit: "Enhance field structure formatting in Ch..." · Reviewed by Greptile |
|
Fixed both issues from the review:
|
Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
| lm: BaseLM, | ||
| lm_kwargs: dict[str, Any], | ||
| signature: type[Signature], | ||
| demos: list[dict[str, Any]], | ||
| inputs: dict[str, Any], | ||
| ) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
The fallback changes leave acall’s parameters without an async def acall( declaration. Python raises a syntax error at line 115, so import dspy fails before any adapter call or test can run. Restore the declaration and correct the method indentation.
Artifacts
Executed adapter parser and import check source
- The authored Python script parses either revision and attempts a real dspy import at head; it is the source used for both captured runs.
Previous review revision parser output
- Ran the check against SHA 9393276 from /home/user/repo with exit code 0; the adapter parsed successfully.
PR head parser and import failure output
- Ran the check against head from /home/user/repo with exit code 1; parsing and importing dspy both fail at chat_adapter.py:115.
This comment has been minimized.
This comment has been minimized.
|
Hi DSPy team! I wanted to follow up on this PR. The conflicts have been resolved, the Greptile review passed, and there are no conflicts with the base branch. The previous CI run expired before a maintainer could approve it. Could someone please approve the workflow so the tests can run? I'm happy to make any additional changes if needed. Thank you! |
Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
|
Hi! The Ruff fix has been applied. There are 2 remaining test failures in test_chat_adapter_native_reasoning and test_json_adapter_native_reasoning — the assertion 'reasoning' not in output_fields is failing. Could a maintainer advise whether this is a pre-existing issue or something I need to address in my changes? Happy to fix it. |
Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
| if not self.field_markers: | ||
| if not signature.output_fields: | ||
| return None | ||
| (f, v), = signature.output_fields.items() | ||
| return f"Respond with the value for `{f}`{type_info(v)}. Do not include any other text." # <-- also inside type_info(), and dedented one level short |
There was a problem hiding this comment.
With field_markers=False, the prompt requests an unmarked answer, but StreamListener still waits for [[ ## field_name ## ]] before emitting field chunks. Passing raw answer chunks through the listener produces no field events, including at finalization, while the same answer with markers streams correctly. The completed raw answer still parses successfully. Teach the listener to stream the sole output without markers, or reject this combination clearly.
Artifacts
Executed isolated streaming validation script
- Extracts and executes the exact relevant HEAD source with real LiteLLM chunks and explicit dependency shells, making the narrow reproduction inspectable and repeatable.
Executed commands and output capture script
- Runs the package import, both comparison cases, and tracked-file integrity check while recording each command, working directory, and exit code.
Package import failure with unchanged source
- Attempts a real DSPy import and captures the syntax error at chat_adapter.py:115, showing why overall integration could not run.
Marker-enabled control streams the answer
- Runs the real listener class with split field markers and answer chunks, showing six field events and successful parsing.
Marker-free answer produces no field events
- Runs the same answer chunks without markers through the real listener class, showing zero field events despite successful completed-answer parsing.
Tracked files remain unchanged
- Runs git diff against HEAD after validation and captures exit code zero, confirming no tracked source was repaired or modified.
Removed an unused line from the test for ChatAdapter. Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
| if not self.field_markers: | ||
| if not signature.output_fields: | ||
| return None | ||
| (f, v), = signature.output_fields.items() | ||
| return f"Respond with the value for `{f}`{type_info(v)}. Do not include any other text." |
There was a problem hiding this comment.
Indentation prevents package loading
The changed if not self.field_markers: line has six leading spaces, which does not match an enclosing indentation level in user_message_output_requirements(). Compiling this method independently raises IndentationError at line 214, so fixing the earlier broken acall declaration alone will still leave the package unable to load. Align this branch with the method body and indent its contents consistently.
Artifacts
- Extracts the unchanged method text from both Git revisions and compiles each inside a minimal class wrapper, isolating the new indentation error.
- Captures the exact command, source, and successful compilation at 901b2e5, showing that the previous method has no indentation parse failure.
- Captures the exact command, source, and IndentationError at head line 214, confirming the new syntax blocker independently.
Refactor format_field_structure method to improve clarity and maintain functionality. Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
| parts = [] | ||
| parts.append("All interactions will be structured in the following way, with the appropriate values filled in.") | ||
|
|
||
| """...""" |
There was a problem hiding this comment.
New indentation prevents package loading
format_field_structure() now has invalid indentation. Its docstring is aligned with the method declaration, and both groups of parts.append() calls are indented without an enclosing block. These additional syntax errors prevent dspy from loading even after the earlier errors are fixed. Indent the docstring inside the method and align each append with its preceding parts = [].
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Updated the format_field_structure method to include a description of the required input and output fields structure. Added a 'completed' field marker to indicate the end of output fields. Signed-off-by: Ramya sree <ramyasreenagarajan@gmail.com>
|
“Hi! I’ve addressed all the Greptile P1 feedback — fixed the UnboundLocalError by moving the helper function above the field_markers branch, restored the parts logic, corrected docstring indentation, and fixed the user_message_output_requirements indentation. The one remaining P1 is the StreamListener streaming issue — I’d appreciate maintainer guidance on the best approach there before implementing. The branch is conflict-free and ready for review. Thank you!” |
Summary
Adds a "field_markers: bool = True" parameter to "ChatAdapter" that, when set to
"False", omits the "[[ ## field_name ## ]]" structural markers from prompts and
completions — reducing token overhead for signatures with a single output field.
Closes #10073
Design notes
"parse()" separate multiple output fields in the raw completion text — without them,
there's no reliable way to split a multi-field completion. Attempting to use
"field_markers=False" with more than one output field raises a clear "ValueError"
at call time (see "_validate_field_markers").
recommended alternative.
Testing
Added two tests:
the prompt and parsing works correctly for a single-output signature.
is raised for multi-output signatures.
All 78 existing tests in "test_chat_adapter.py" continue to pass.