Skip to content

fix(dspy): discover predictors nested inside containers in named_parameters - #10152

Open
anxkhn wants to merge 2 commits into
stanfordnlp:mainfrom
anxkhn:fix/nested-parameter-discovery
Open

anxkhn wants to merge 2 commits into
stanfordnlp:mainfrom
anxkhn:fix/nested-parameter-discovery

Conversation

@anxkhn

@anxkhn anxkhn commented Aug 6, 2026

Copy link
Copy Markdown

What

BaseModule.named_parameters only traversed one level of list, tuple, and dict attributes. Containers nested inside another container were never entered, so any predictor stored below the first level was invisible to the rest of the framework.

Reproduction

import dspy

class Program(dspy.Module):
    def __init__(self):
        super().__init__()
        self.groups = [[dspy.Predict("question -> answer")]]

program = Program()
print(program.named_predictors())  # [] -- expected [("groups[0][0]", Predict(...))]
print(program.dump_state())        # {} -- expected the nested predictor's state

The program runs fine when the predictor is called directly, but optimizers see no predictors to optimize, dump_state/load_state silently drop it, reset_copy does not reset it, and set_lm does not configure it. named_sub_modules already walks arbitrarily nested containers, so the two traversals disagreed.

Change

named_parameters now uses a single recursive walker over Parameter, dspy.Module, list, tuple, and dict values instead of a one-level loop plus a two-case helper. Behaviour that was already there is preserved:

  • shared references are still deduplicated by object identity, first path wins
  • pre-compiled sub-modules are still skipped and kept frozen
  • path formatting is unchanged (name.sub, name[0], name['key']) and now matches named_sub_modules for nested cases

The walker carries one visited set seeded with the module itself, so reference cycles (a module reachable from its own attributes, or a self-referencing dict) terminate instead of recursing forever. Sub-modules are traversed through their __dict__ rather than by re-entering named_parameters, which is what makes the shared visited set and the cycle guard work across module boundaries.

Tests

Added a regression in tests/primitives/test_module.py covering a predictor buried in [[{...}]] with a duplicated reference and a self-referencing dict, and asserting the discovered path plus dump_state, load_state, reset_copy, and set_lm all reach it.

pytest tests/primitives/test_module.py tests/primitives/test_base_module.py

AI disclosure

An AI coding assistant was used to draft this patch and its test, under human review; the change and the test output were checked by hand before submitting. Prompt used:

Fix dspy/primitives/base_module.py so named_parameters recursively discovers predictors nested inside lists, tuples, and dicts (for example self.groups = [[dspy.Predict("q -> a")]]). Keep deduplication of shared references, keep skipping pre-compiled sub-modules, make the traversal cycle-safe, and keep the path format consistent with named_sub_modules. Add a regression test in tests/primitives/test_module.py covering nested containers, shared references, cycles, state round-trip, and set_lm.

@greptile-apps

greptile-apps Bot commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Recursive parameter discovery now reaches predictors in nested containers. However, predictors stored under tuple dictionary keys cannot be replaced through map_named_predictors(), so programs using tuple-keyed predictor registries fail during remapping.

Confidence Score: 4/5

Not ready to merge until predictor remapping supports every dictionary-key path emitted by recursive parameter discovery.

A reproduced runtime failure prevents tuple-keyed predictors from being remapped.

Files Needing Attention: dspy/primitives/base_module.py

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for a posted P1 finding and linked it to the corresponding review comment.
  • T-Rex performed general contract validation, capturing a NotImplementedError from a tuple-key run and verifying that the original predictor remained in the dictionary after the exception.
  • T-Rex produced a second proof for another posted P1 finding, linked to the review comment.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 map_named_predictors cannot replace predictors stored under tuple dictionary keys

    • Bug
      • BaseModule.named_parameters() emits the path predictors[(1, 2)] for a predictor stored under tuple key (1, 2). Module.map_named_predictors() passes that path to magicattr.set(), whose subscript parser rejects the AST Tuple; it raises NotImplementedError and the predictor is left unchanged.
    • Cause
      • Dictionary keys are stringified with {key!r} when names are generated, but magicattr._lookup_subscript_value() only supports constants and negative numeric literals, not tuple AST nodes.
    • Fix
      • Make the generated parameter path format supported by the setter for arbitrary dictionary keys, or extend magicattr subscript-value parsing to safely evaluate tuple literal keys (including nested supported literals).

    T-Rex Ran code and verified through T-Rex

Reviews (2): Last reviewed commit: "fix(dspy): preserve dictionary keys in p..." | Re-trigger Greptile

Comment thread dspy/primitives/base_module.py Outdated
@anxkhn
anxkhn force-pushed the fix/nested-parameter-discovery branch from aea0b7e to c9a5d22 Compare August 11, 2026 21:33
@anxkhn

anxkhn commented Aug 11, 2026

Copy link
Copy Markdown
Author

fixed dictionary key path serialization so integer and quote-containing keys round-trip through map_named_predictors, with regression coverage for both cases. rebased onto the latest main; the focused tests and ruff pass. a maintainer review would be appreciated.

add_parameter(f"{param_name}[{idx}]", item)
elif isinstance(param_value, dict):
for key, item in param_value.items():
add_parameter(f"{param_name}[{key!r}]", item)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Tuple-key predictor paths cannot be mapped

named_parameters() renders a predictor stored under a tuple dictionary key such as (1, 2) as predictors[(1, 2)]. map_named_predictors() passes that name to magicattr.set(), whose subscript parser rejects tuple AST nodes with NotImplementedError; mapping aborts and leaves the original predictor in place. Use a reversible path representation supported by the setter, or extend the setter to handle tuple dictionary keys.

Artifacts

Focused tuple-key predictor reproduction script source

  • Captured the exact authored Python script that creates a DSPy module with a predictor in a dictionary and runs the mapping path, showing the test setup used for the execution.

Successful string-key predictor replacement control output

  • Captured `uv run` output for the equivalent string-key dictionary case, showing that naming and predictor replacement succeed.

Tuple-key predictor replacement failure output

  • Captured `uv run` output for tuple key `(1, 2)`, showing the generated name, `NotImplementedError` from magicattr, and that replacement was not applied.

View artifacts

T-Rex Ran code and verified through T-Rex

This branch has not been deployed

No deployments
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.

1 participant