Skip to content

Conversation

@rock-n-shrimproll
Copy link
Collaborator

@rock-n-shrimproll rock-n-shrimproll commented Jan 27, 2025

Description

Please describe here what changes are made and why.

Checklist

  • I have performed a self-review of the changes

List here tasks to complete in order to mark this PR as ready for review.

To Consider

  • Add tests (if functionality is changed)
  • Update API reference / tutorials / guides
  • Update CONTRIBUTING.md (if devel workflow is changed)
  • Update .ignore files, scripts (such as lint), distribution manifest (if files are added/deleted)
  • Search for references to changed entities in the codebase

@RLKRo RLKRo added the enhancement New feature or request label Mar 24, 2025
@rock-n-shrimproll rock-n-shrimproll requested a review from RLKRo March 31, 2025 14:15
@rock-n-shrimproll rock-n-shrimproll self-assigned this Apr 2, 2025
Comment on lines +67 to +71
response_exception: Optional[str] = Field(default=None, exclude=True)
"""
Stores exception messages raised from response functions wrapped in
:py:class:`~chatsky.processing.standard.AddFallbackResponses`.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I think that this is too random of a field that is unusable in many cases.

Better approach seems to be to make this field hold last exception raised from any script function and not only responses when AddFallbackResponses is used.

class BaseScriptFunction:
  async def wrapped_call(ctx):
    ...
    except Exception as exc:
      ctx.framework_data.last_exception = exc

Preferably in a separate PR.

"Enables complex stats collection across multiple turns."
slot_manager: SlotManager = Field(default_factory=SlotManager)
"Stores extracted slots."
response_exception: Optional[str] = Field(default=None, exclude=True)
Copy link
Member

Choose a reason for hiding this comment

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

Do you remember why this holds repr instead of exception?
Seems like it's better to store exception since this field will not be serialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants