-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Increased test coverage for ChatAdapter #8168
Conversation
There was a problem hiding this 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.
tests/adapters/test_chat_adapter.py
Outdated
subsubfield1: str = pydantic.Field(description="String subsubfield") | ||
subsubfield2: int = pydantic.Field(description="Integer subsubfield", ge=0, le=10) | ||
|
||
class InputField(pydantic.BaseModel): |
There was a problem hiding this comment.
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?
dspy/tests/adapters/test_json_adapter.py
Line 110 in ffb415f
def test_json_adapter_on_pydantic_model(): |
There was a problem hiding this comment.
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!
There was a problem hiding this 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?
85c07d2
to
e77f275
Compare
Rebased, everything is clean |
@JHMuir thanks! Looks like there is a failed test case:
Does it pass locally for you? |
There was a problem hiding this 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!
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 :) |
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.