Repository navigation
Conversation
Contributor
[Low impact] The PR appears safe to merge; both previous findings are fixed and no new actionable issues remain. Summary
Reviews (2) · Last reviewed commit: "fix(predict): materialize default_factor..." · Reviewed by Greptile |
Populate missing inputs with FieldInfo.is_required() and materialize factory fields via get_default(call_default_factory=True). Literal defaults keep their existing value and identity. Pass the values supplied so far as validated_data so factories that accept an input dict work, and enable logger propagation in the warning regression test. Factory-backed inputs were previously never populated, so the adapter omitted them from the prompt and the missing-input warning fired even though the field had a default.
BlueX888
force-pushed
the
contrib/input-default-factory
branch
from
November 3, 2026 08:34
29f51cf to
6bcc0c6
Compare
Author
|
Addressed both findings in 6bcc0c6:
Verified: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Changes Description
Predictpopulated missing input defaults only whenFieldInfo.defaultwas notPydanticUndefined. For an input declared withdefault_factory,.defaultis undefined, so the factory was never called: the field was absent from the prompt, and the module loggedNot all input fields were provided ... Missing: ['context']even though the field had a default. The output-field path already materializes factories throughget_default(call_default_factory=True).This change populates missing inputs with
FieldInfo.is_required()and materializes factory fields viaget_default(call_default_factory=True), passing the raw input values supplied so far asvalidated_datafor factories that accept an input dict (DSPy does not validate inputs, so this mirrors the available values rather than pydantic model validation). Literal defaults keep their existing value and identity; required-field warnings, optional-annotation behavior, and output defaults are unchanged. The unusedPydanticUndefinedimport is removed.A related PR (#10034) made an equivalent Predict change in a stacked feature branch (
isaac/rlm-namespace-collisions); it is not in current main, which still has the old code (verified againstc8176a8).✅ Contributor Checklist
pytest tests/predict614 passed,ruff checkclean,ty checkwith the same 16 pre-existing diagnostics as base)fix(predict): ...)fix(predict): ...)Tests use the existing
spy_lmhelper and the default adapter; the fix is covered throughPredict._forward_preprocess, which is shared by forward and async paths.