Skip to content

Increasing test coverage for JsonAdapter #8131

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 9 commits into from
May 3, 2025

Conversation

Y-1huadb
Copy link
Contributor

This PR adds three additional test cases for the JsonAdapter:

  • Handling Different Input and Output Field Types: The test ensures that the JSONAdapter correctly processes various input and output field types, including nested Pydantic models and complex data structures. It verifies that the adapter handles both simple and nested fields in the expected manner when making requests to the model.
  • Passing Signature Information to the Model: This test verifies that the request sent to the language model (LM) includes the correct signature information. It checks that the input and output fields, along with their descriptions and types, are correctly embedded in the request, ensuring the LM receives the signature as intended.
  • Raising Explicit Errors on Failure: The test ensures that when parsing errors or mismatches between expected and actual field keys occur, the JSONAdapter raises a clear and explicit error. Specifically, if the model's output does not align with the expected fields defined in the signature, a ValueError is raised with an appropriate message. This helps prevent silent failures and provides clear feedback when something goes wrong during parsing.

All tests could be passed locally.

If there are any mistakes in my attempt, I sincerely apologize and appreciate you taking the time to review my pull request. I have great respect for your work. Thanks.

@Y-1huadb
Copy link
Contributor Author

Fixes: #8121

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.

Thanks for the PR! left some comments to simplify the test

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.

Thanks for fixing! Overall LGTM, I will take over the cleanup and merge it. Thanks again for the conbribution!

@chenmoneygithub chenmoneygithub merged commit 9272605 into stanfordnlp:main May 3, 2025
3 checks passed
@Y-1huadb Y-1huadb deleted the 'unit_test_json_adapter' branch May 3, 2025 03:16
@Y-1huadb
Copy link
Contributor Author

Y-1huadb commented May 3, 2025

I'm glad I could help. Thanks for handling the cleanup and merge!

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