- 
                Notifications
    
You must be signed in to change notification settings  - Fork 134
 
chore(weave): Add serialization assertions for the remaining data types: Evaluation Library Types (4) #5578
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
Conversation
          Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know!  | 
    
| 
           Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=f1b508750215c74f62096b785181e0b8194ae900  | 
    
| # Sad ... equality is really a pain to assert here (and is broken) | ||
| # TODO: Write a good equality check and make it work | ||
| equality_check=lambda a, b: True, | 
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.
Maybe this should be fixed before merging
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.
I agree. However, I am really trying to just get the current expectations fixed so that I can modify for a customer.
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.
Maybe I am missing something -- isn't the assert the expectation? How can you be sure unless the test actually passes?
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.
oh, the equality check is just one of many assertions. we do a bunch of validation on the actual payloads as well.
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 test the library objects individually as well as composites
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.
Approving despite lack of asserts :)
Adds Evaluation library objects