Skip to content

fix: allow reasoning traces when embeddings_only is True#1170

Merged
Pouyanpi merged 1 commit intodevelopfrom
fix/reasoning-traces-validation-embedding-only
May 5, 2025
Merged

fix: allow reasoning traces when embeddings_only is True#1170
Pouyanpi merged 1 commit intodevelopfrom
fix/reasoning-traces-validation-embedding-only

Conversation

@Pouyanpi
Copy link
Copy Markdown
Collaborator

@Pouyanpi Pouyanpi commented May 5, 2025

Refactors the dialog rails validation logic to properly respect the embeddings_only flag, allowing reasoning traces to be enabled when this flag is set to True. Adds tests to verify this behavior.

related to #1161 and #1137

Refactors the dialog rails validation logic to properly respect the embeddings_only flag, allowing reasoning traces to be enabled when this flag is set to True. Adds tests to verify this behavior.
@Pouyanpi Pouyanpi force-pushed the fix/reasoning-traces-validation-embedding-only branch from 3e92060 to a5ae1ca Compare May 5, 2025 08:44
@Pouyanpi Pouyanpi requested a review from trebedea May 5, 2025 08:44
@Pouyanpi Pouyanpi added this to the v0.14.0 milestone May 5, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.06%. Comparing base (d240625) to head (a5ae1ca).
⚠️ Report is 64 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1170   +/-   ##
========================================
  Coverage    68.05%   68.06%           
========================================
  Files          161      161           
  Lines        15820    15822    +2     
========================================
+ Hits         10767    10769    +2     
  Misses        5053     5053           
Flag Coverage Δ
python 68.06% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
nemoguardrails/rails/llm/config.py 89.83% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Pouyanpi Pouyanpi requested review from Copilot and removed request for trebedea May 5, 2025 09:42
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the reasoning traces validation logic to allow reasoning traces when the embeddings_only flag is True and adds tests to verify this behavior.

  • Updates validation in RailsConfig to bypass dialog rails checks when embeddings_only is enabled.
  • Adds new tests for scenarios where embeddings_only is set for both user and bot message flows.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/test_config_validation.py Updates tests and adds new test cases to verify embeddings_only behavior.
nemoguardrails/rails/llm/config.py Refactors dialog rails validation logic to respect the embeddings_only flag.
Comments suppressed due to low confidence (2)

tests/test_config_validation.py:619

  • [nitpick] The test function's docstring mentions bot messages, but the YAML configuration only specifies 'user_messages' with embeddings_only set. Clarify the test either by updating the YAML to include bot_messages or revising the docstring to match the configuration.
def test_reasoning_traces_with_bot_messages_embeddings_only():

nemoguardrails/rails/llm/config.py:1210

  • [nitpick] The embeddings_only flag is only retrieved from the user_messages configuration. If there is an intention to support embeddings_only for bot messages as well, consider updating the logic accordingly or documenting the limitation.
embeddings_only = dialog_rails.get("user_messages", {}).get("embeddings_only", False)

@Pouyanpi Pouyanpi self-assigned this May 5, 2025
Copy link
Copy Markdown
Member

@trebedea trebedea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Pouyanpi
Copy link
Copy Markdown
Collaborator Author

Pouyanpi commented May 5, 2025

Looks good!

Thank you @trebedea !

@Pouyanpi Pouyanpi merged commit 5b5fea0 into develop May 5, 2025
39 checks passed
@Pouyanpi Pouyanpi deleted the fix/reasoning-traces-validation-embedding-only branch May 5, 2025 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants