-
Notifications
You must be signed in to change notification settings - Fork 53
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
patch atexit_serde_create_op_test #2842
Conversation
rdspring1
commented
Aug 24, 2024
- Failed to run correctness tests because the scope member was not initialized correctly.
!build |
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 with minor nitpick
4dd6f98
to
aa9e9cd
Compare
* Failed to run correctness tests because the scope member was not initialized correctly.
795255d
to
ffcd96e
Compare
!build |
@jjsjann123 I removed the per-op serialization check because it was making the CI take too long. Currently, the python tests take about an hour. With the serialization check, it is over 1 hour, 45 minutes. We'll just run the serialization check at exit for the opinfo tests in the CI. |
naive question, are the two checks identical? If so, should we at least run serde_check_ops in our nightly/weekly runs to ensure coverage? |
You're correct. The opinfo tests covers each op individually and handles more inputs, so it is slower. The opinfos are useful for development or debugging because every op is isolated.
That is a good idea. If you have unlimited time it doesn't hurt to run them, but |