Skip to content
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

Revert "Fix mypy typing errors in pytorch_lightning/strategies/single_tpu.py" #13544

Closed
wants to merge 1 commit into from
Closed

Revert "Fix mypy typing errors in pytorch_lightning/strategies/single_tpu.py" #13544

wants to merge 1 commit into from

Conversation

CyprienRicque
Copy link
Contributor

@CyprienRicque CyprienRicque commented Jul 5, 2022

Reverts #13534

In the previous PR I explained that the condition is_overridden("on_post_move_to_device", self.lightning_module) can not be True. This is incorrect. I missed the heritage of LightningModule with ModelHooks.
Indeed, ModelHooks implements on_post_move_to_device.
I am very sorry for this.

I think I did not see it in the unit tests because this function has been deprecated in v1.5 and will be removed in v1.7, therefore it is not tested. According to this test: https://github.com/Lightning-AI/lightning/blob/61c28cb428a13c2aea6d7f3f55e0f00431a4ea4e/tests/tests_pytorch/deprecated_api/test_remove_1-7.py#L64

@justusschock
Copy link
Member

Probably no need to revert it, as #13534 will be merged to 1.7 only and won't be included in 1.6.X

@akihironitta
Copy link
Contributor

@Cyprien-Ricque Thank you for your action! The change shouldn't have been part of your previous PR #13534, but as mentioned above, there's no need to revert the change because my PR #13548 will remove it anyway as part of #12521 and the change won't be included in 1.6.x. Could you close this PR?

@awaelchli awaelchli added the won't fix This will not be worked on label Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
won't fix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants