Skip to content

Conversation

Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Oct 1, 2025

Stack Info

This PR is part of a stack:

  1. refactor!: drop reasoning trace extraction logic #1427
  2. feat: add reasoning trace extraction from llm calls #1431 (this PR)
  3. feat: emit BotThinking events with reasoning traces #1432
  4. feat(logging): improve event logging and add bot thinking display #1434

⚠️ Depends on: #1427

Description

add reasoning trace extraction from llm calls as we are using invoke and ainvoke in llm_call

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/actions/llm/utils.py 66.66% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Collaborator

@tgasser-nv tgasser-nv 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, just a question on the refactoring intent

): # Exclude content since it may be modified by rails
metadata[field_name] = getattr(response, field_name)
llm_response_metadata_var.set(metadata)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand if you're refactoring here to clean things up, shouldn't we call _store_reasoning_trace() to maintain the same functionality ?

return None


def extract_bot_thinking_from_events(events: list):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should this be called 'extract_first_bot_thinking_from_events()' or similar? If the events are ordered with the newest at the start of the list this returns the first one

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.

3 participants