-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Replace occurrences of on_before_accelerator_backend_setup_called with setup hook #11568
Replace occurrences of on_before_accelerator_backend_setup_called with setup hook #11568
Conversation
on_before_accelerator_backend_setup
on_before_accelerator_backend_setup
on_before_accelerator_backend_setup
hook
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 working on this @krishnakalyan3
I'd make this into 2 PRs:
- Transition all Lightning callbacks to use
setup
instead ofon_before_accelerator_backend_setup
. These callsites are shared in the initial issue - Mark the callback hook as deprecated (adding to the config validator, change the docstring of the hook to reflect it's deprecated, etc)
on_before_accelerator_backend_setup
hookon_before_accelerator_backend_setup
hook [1/2]
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.
Since this is a minor change do I need to update the change logs?. |
A few comments:
|
@daniellepintz Let me know if the new changes look good. |
for more information, see https://pre-commit.ci
@daniellepintz let me know if this looks 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.
LGTM! Please fix the lint errors. You can check lint locally by running pre-commit
(https://pre-commit.com/)
@awaelchli let me know if the new changes look okay?. |
Looks like the pre-commit messages are picking up this from this signature Any suggestions on how to resolve this?
|
add |
@rohitgr7 since |
@krishnakalyan3 |
What does this PR do?
Part-1/2 #11559
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃