fix(dspy): Fix Module.__call__ to enable nested module usage tracking with track_usage#9213
fix(dspy): Fix Module.__call__ to enable nested module usage tracking with track_usage#9213TomeHirata with Copilot wants to merge 9 commits into
Conversation
Modified _set_lm_usage() to handle lists of Predictions, not just single Prediction or tuple Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
Extended _set_lm_usage to handle tuple containing list of Predictions, such as ([Prediction, ...], trace) Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
Added checks to ensure lists are non-empty before iterating with all(), improving performance Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
|
@copilot I think this fix is fragile as there are many other types that users' custom modules may return. It it difficult to set usage for all module output when it returns |
Changed from type-specific checks to a recursive function that finds all Prediction objects in any structure (lists, tuples, dicts, nested combinations). This makes the implementation more robust and future-proof for user-defined return types. Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
Good point! I've refactored the implementation to use a recursive approach that finds all The new implementation traverses the entire output structure recursively and sets usage on all Fixed in commit adc30e3. |
Added detailed docstring explaining parameters, behavior, and side effects. Clarified circular reference prevention logic. Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
| prediction_in_output = output[0] | ||
| if prediction_in_output: | ||
| prediction_in_output.set_lm_usage(tokens) | ||
| # When usage tracking is enabled, recursively find all Prediction objects |
There was a problem hiding this comment.
@copilot I think we should fix Module.__call__ instead of _set_lm_usage. Currently, _set_lm_usage is called only on the root module call and no usage is added to the nested module call.
There was a problem hiding this comment.
You're absolutely right! I've fixed Module.__call__ to handle nested module calls. Previously, only the root module would call _set_lm_usage. Now when track_usage is enabled:
- If no tracker exists: creates one and sets usage (root module case)
- If tracker exists: captures token delta before/after
forward()and sets usage on output (nested module case)
This ensures all module calls (root and nested) properly track and set usage on their Prediction outputs.
Fixed in commit e8c0ee0.
There was a problem hiding this comment.
@copilot do we still need the change for _set_lm_usage?
There was a problem hiding this comment.
Yes, we still need the recursive _set_lm_usage because both fixes address different concerns:
1. Module.call fix (commit e8c0ee0): Ensures each module call (root or nested) gets its usage tracked
- Handles:
OuterModule()→ callsinner_module()→ both get usage set
2. Recursive _set_lm_usage (commits 7f381e8-adc30e3): Ensures all Predictions in a single module's output get usage set
- Handles: Module returns
[Prediction, Prediction]fromdspy.Parallel→ all get usage set - Original only handled single Prediction or tuple, so lists would get the warning and no usage
The original bug was a module returning a list of Predictions from Parallel. Without the recursive _set_lm_usage, even with the call fix, the list wouldn't be traversed and individual Predictions wouldn't get usage set.
Both fixes are needed for the complete solution.
Modified Module.__call__ to always call _set_lm_usage when track_usage is enabled, even for nested module calls. Previously, only the root module call would set usage. Now nested modules also get their usage set by computing the delta between token counts before and after the forward call. Added _compute_token_delta helper method to calculate the difference in token usage. Added test for nested module usage tracking. Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
- Removed redundant usage_tracker None check in nested call branch - Handle all LM names from both before and after token snapshots in delta calculation - Improved _compute_token_delta to handle non-numeric values and process all keys from both dicts - Added type checking to prevent TypeError on non-numeric types Co-authored-by: TomeHirata <33407409+TomeHirata@users.noreply.github.com>
📝 Changes Description
dspy.Parallelreturns a list ofPredictionobjects, butModule._set_lm_usage()only handled singlePredictionor tuple cases. Additionally, nested module calls did not track usage - only the root module would set usage on its output.This PR fixes both issues with complementary changes that address different concerns:
_set_lm_usage()to use a recursive approach that finds allPredictionobjects in any output structure, making it robust and future-proof for user-defined custom return typesModule.__call__()to properly handle nested module calls by computing token usage deltas for each module invocationWhy both fixes are needed:
Module.__call__fix ensures each module call (root or nested) tracks its usage_set_lm_usageensures allPredictionobjects within a single module's output get usage set (e.g., when returning lists/dicts/nested structures)dspy.Parallelrequires both fixes to work correctlyChanges:
_set_lm_usagewhentrack_usageis enabled:collect_predictions()function that traverses any data structurePrediction, lists/tuples, nested structures (e.g.,[[Prediction, ...], ...]), dicts withPredictionvalues, and any combinationBenefits:
✅ Contributor Checklist
None. The implementation properly handles both root and nested module usage tracking. All existing tests pass, plus new tests validate nested modules, nested structures, and edge cases. Both fixes (Module.call and recursive _set_lm_usage) are necessary and address orthogonal concerns.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.