-
Notifications
You must be signed in to change notification settings - Fork 617
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
Update device tests to work with Lightning devices #5518
Conversation
Hello. You may have forgotten to update the changelog!
|
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.
Thanks @mudit2812! 🙌
[sc-61419] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5518 +/- ##
==========================================
- Coverage 99.67% 99.66% -0.01%
==========================================
Files 404 404
Lines 37852 37561 -291
==========================================
- Hits 37728 37436 -292
- Misses 124 125 +1 ☔ View full report in Codecov by Sentry. |
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.
Thanks @mudit2812
Quick question:
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'm approving this now because it's blocking lightning, but we should update validate_diff_method
to actually check dev.supports_derivatives
instead of adding a lightning patch. Otherwise we will keep trying to test backprop against devices that don't support it.
@albi3ro I've made the suggested changes. |
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.
Thanks for making that change 👍
@mudit2812 was this intended to fix #5519? |
The new tests added to the device test suite weren't totally compatible with lightning devices. I've updated the failing tests to work now. The tensorflow tests failed because our execution pipeline with tensorflow doesn't correctly respect the dtypes, which is a larger issue for us to resolve, so it will be handled in a different PR. For now I've skipped the failing tests.