-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Move sync code from step result to lightning module [6/n] #7651
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
@@ Coverage Diff @@
## master #7651 +/- ##
======================================
Coverage 92% 92%
======================================
Files 198 198
Lines 12809 12842 +33
======================================
+ Hits 11810 11842 +32
- Misses 999 1000 +1 |
@staticmethod | ||
def __sync( |
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.
why not move this out of the LighningModule completely to a utilities file?
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.
Do you think other places might use it?
Do you have an idea of where it would fit better?
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 think it could be part of #7534
But this is definitely not a blocker for this refactoring
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.
Sure, feel free to include it when that's done
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 !
What does this PR do?
Part of #7183
Before submitting
PR review