Skip to content
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 assertion bug with new_signature #1372

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

Shangyint
Copy link
Collaborator

Co-authored-by: chiragshah285
@chiragshah285

@Shangyint
Copy link
Collaborator Author

@arnavsinghvi11 Would there be an issue if we change target_module to be the actual module in Assertions854a259? It seems assert_transform will re-assign the wrapped Retry module to all modules defined in __init__ thus we shouldn't have the problem?

@arnavsinghvi11
Copy link
Collaborator

Thanks @chiragshah285 @Shangyint. This change makes sense, but will require some refactoring to existing assertion notebooks examples as the assertion statements currently rely on the target_module mapped to corresponding module signature. I believe this logic was kept originally as with 2 assertion statements referring to the same Module type (e.g. ChainOfThought), Retry was not identifying the correct module to backtrack on and hence relied on the signature identifier.

will require some further testing to verify if the proposed logic correctly maintains this. Feel free to check out an example notebook using assertions (LongFormQA - particularly where it uses dspy.Suggest on target_module=GenerateCitedParagraph while the module includes 2 chain of thought layers, dspy.ChainOfThought(GenerateSearchQuery) and dspy.ChainOfThought(GenerateCitedParagraph)

@okhat
Copy link
Collaborator

okhat commented Aug 19, 2024

What's the urgency and status here?

@okhat okhat merged commit b7435c1 into main Sep 9, 2024
5 checks passed
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.

3 participants