Repository navigation
Conversation
Greptile SummaryRecursive parameter discovery now reaches predictors in nested containers. However, predictors stored under tuple dictionary keys cannot be replaced through Confidence Score: 4/5Not 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
What T-Rex did
|
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
aea0b7e to
c9a5d22
Compare
|
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) |
There was a problem hiding this comment.
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.
What
BaseModule.named_parametersonly traversed one level oflist,tuple, anddictattributes. 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
The program runs fine when the predictor is called directly, but optimizers see no predictors to optimize,
dump_state/load_statesilently drop it,reset_copydoes not reset it, andset_lmdoes not configure it.named_sub_modulesalready walks arbitrarily nested containers, so the two traversals disagreed.Change
named_parametersnow uses a single recursive walker overParameter,dspy.Module,list,tuple, anddictvalues instead of a one-level loop plus a two-case helper. Behaviour that was already there is preserved:name.sub,name[0],name['key']) and now matchesnamed_sub_modulesfor nested casesThe walker carries one
visitedset 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-enteringnamed_parameters, which is what makes the sharedvisitedset and the cycle guard work across module boundaries.Tests
Added a regression in
tests/primitives/test_module.pycovering a predictor buried in[[{...}]]with a duplicated reference and a self-referencing dict, and asserting the discovered path plusdump_state,load_state,reset_copy, andset_lmall reach it.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: