-
Notifications
You must be signed in to change notification settings - Fork 549
refactor!: drop reasoning trace extraction logic #1427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…al_kwargs BREAKING CHANGE: Remove reasoning_config and apply_to_reasoning_traces config options - Remove ReasoningModelConfig, ParsedTaskOutput, and related extraction logic - Replace with automatic extraction from response.additional_kwargs['reasoning_content'] - Remove 1,085 lines of test code and related infrastructure - Simplify parse_task_output() to return str directly - Update DeepSeek-R1 and Nemotron example configs Reasoning traces are now automatically extracted by LangChain and stored in additional_kwargs, eliminating the need for token-based parsing and complex configuration options.
c4d3366
to
8aee801
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just want to check we cover lines 201-3 of actions/llm/utils.py in another of the PRs in the stack
isinstance(additional_kwargs, dict) | ||
and "reasoning_content" in additional_kwargs | ||
): | ||
reasoning_content = additional_kwargs["reasoning_content"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 201-203 aren't covered by tests in this PR. These are pretty important since they're where we pull out the additional_kwargs
and put them in the context var (!) Are there tests in another one of the stacked PRs that cover these?
@Pouyanpi , shouldn't we keep |
Not anymore, now if someone wants to use a specific output rails that checks the reasoning content should include it in the prompt template (see https://github.com/NVIDIA-NeMo/Guardrails/pull/1432/files#diff-1eb0f307d462ecfbe7710b361f4d296d3648a44bfcf85fda895c6942b2e4606dR37-R39 ) and use a rail that supports guardrailing reasoning contents ( for example see how actions.py is modified) There needs to be a subsequent PR that updates some of the actions. |
I don't have anything against it, but for me it seemed like a useful option to be able to run any output rails on response only vs (thinking + response) by only adding a config flag. I agree that with the current implementation even more complex output rails can be defined way easier, but maybe it's not a bad idea to have the flag anyway. |
Stack Info
This PR is part of a stack:
Description
Replaces custom reasoning trace extraction with LangChain's built-in
additional_kwargs
approach, removing ~1,500 lines of code.Breaking Changes
reasoning_config
(includingstart_token
,end_token
,remove_reasoning_traces
) removed from model configurationapply_to_reasoning_traces
removed from output rails configurationMigration
Remove
reasoning_config
sections from config files. Reasoning traces are now automatically extracted by LangChain for supported models (e.g., DeepSeek-R1).Changes
ReasoningModelConfig
,ParsedTaskOutput
, token extraction logicresponse.additional_kwargs['reasoning_content']