Skip to content

Increased test coverage for ChatAdapter #8168

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

Merged
merged 7 commits into from
May 21, 2025

Conversation

JHMuir
Copy link
Contributor

@JHMuir JHMuir commented May 4, 2025

Fixes for #8120

Adds 3 new tests to ChatAdapter:

  • test_chat_adapter_with_pydantic_models(): Validates and ensures that ChatAdapter can use Pydantic models as inputs and outputs, nested or basic

  • test_chat_adapter_signature_information(): Ensures that the signature information sent with ChatAdapter follows an expected structure and format

  • test_chat_adapter_exception_raised_on_failure() : Ensures that a specific error is called upon failure

All tests were passed. If there are any changes you'd like me to make, please let me know! More than happy to contribute.

Copy link
Collaborator

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Left a comment on potential improvements.

subsubfield1: str = pydantic.Field(description="String subsubfield")
subsubfield2: int = pydantic.Field(description="Integer subsubfield", ge=0, le=10)

class InputField(pydantic.BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid defining class of name InputField. Although this is just unit test, our users could use it as sample code to follow.

Would you mind refactoring the test case following the json adapter example?

def test_json_adapter_on_pydantic_model():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed some small changes, lmk how they look!

Copy link
Collaborator

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

code looks good to me! Would you mind rebasing from main branch so that we can merge?

@JHMuir JHMuir force-pushed the unit-test-chat-adapter branch from 85c07d2 to e77f275 Compare May 21, 2025 00:28
@JHMuir
Copy link
Contributor Author

JHMuir commented May 21, 2025

Rebased, everything is clean

@chenmoneygithub
Copy link
Collaborator

@JHMuir thanks! Looks like there is a failed test case:

FAILED tests/adapters/test_chat_adapter.py::test_chat_adapter_exception_raised_on_failure - dspy.utils.exceptions.AdapterParseError: Adapter ChatAdapter failed to parse the LM response. 

LM Response: {'output':'mismatched value'} 

Expected to find output fields in the LM response: [answer] 

Actual output fields parsed from the LM response: []
= 1 failed, 362 passed, 194 skipped, 2 xfailed, 17 warnings in 87.31s (0:01:27) =

Does it pass locally for you?

Copy link
Collaborator

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@chenmoneygithub chenmoneygithub merged commit 1699000 into stanfordnlp:main May 21, 2025
3 checks passed
@JHMuir
Copy link
Contributor Author

JHMuir commented May 21, 2025

Ah yeah I didn't see that ChatAdapter added a new explicit error... I was about to push my quick change when I saw you already did. Thank you for merging and all your help :)

@chenmoneygithub
Copy link
Collaborator

@JHMuir No problem! Btw I think you are the contributor #300 of DSPy 😄

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.

2 participants