-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix the crash when usage tracker is enabled with non-prediction output #8831
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
Fix the crash when usage tracker is enabled with non-prediction output #8831
Conversation
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.
Pull Request Overview
This PR fixes a crash that occurs when usage tracker is enabled for dspy modules that return non-dspy.Prediction
outputs. The fix allows such modules to execute normally by skipping usage tracking when no prediction object is found.
- Replaces error condition with conditional check to prevent crashes
- Adds test coverage for modules returning non-prediction outputs with usage tracking enabled
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
dspy/primitives/module.py | Removes error condition and adds conditional check for prediction object before setting LM usage |
tests/primitives/test_base_module.py | Adds test case to verify usage tracker doesn't crash with non-prediction outputs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
I want to discuss a bit about this - I am not sure if we want to change the current behavior to silence the feature without user awareness. track_usage
is an opt-in feature, users need to explicitly turn it on to enable the feature. The drawback of this change is users will see no usage being reported, while no error is raised. There are two patterns we can follow:
- Keep the old behavior, but make the error message better.
- Ship this PR, but log a warning when skipping setting the usage.
Usually I prefer giving users 100% awareness, but I assume you find this being annoying, which also makes sense to me. Let me know your thoughts!
We shouldn't break program execution due to the failure of observability feature. I've added warning so that users can know why usage is not available. |
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.
LGTM with a comment!
Currently, dspy modules whose output is not
dspy.Prediction
fail to execute when the usage tracker is enabled. This PR fixes the crash by skipping the usage tracking for this type of modules. While returning objects other than dspy.Prediciton is not expected, this pattern has been found in several places.Example: