-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add _gpus_arg_default in argparse_utils for backward compatibility #7402
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 #7402 +/- ##
=======================================
- Coverage 92% 87% -4%
=======================================
Files 200 200
Lines 12982 12983 +1
=======================================
- Hits 11908 11358 -550
- Misses 1074 1625 +551 |
carmocca
left a comment
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 a better solution is to set
__all__ = ['_gpus_arg_default'] in pytorch_lightning.utilities.argparse
since this file will be eventually removed
I thought of that solution, but the |
I know, but I would say it's nicer to have both things together.
But if we remove this file in v1.4, will a checkpoint created before 1.2 be able to reload? As you said:
|
|
In future versions, before loading the checkpoint we need to monkey patch a dummy function onto the modules where it was used. The fact that this function got pickled into the checkpoints means we will forever have to track changes surrounding that. Horrific. :( |
tchaton
left a comment
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 !
That is the real problem. Both of the solutions won't help when this file is removed. |
What does this PR do?
Fixes #7400.
pytorch_lightning/utilities/argparse_utils.pywas renamed topytorch_lightning/utilities/argparse.pyat PL 1.2.0.A checkpoint saved in PL < 1.2 tries to use argparse.utils._gpus_arg_default() which does not exists in PL >= 1.2.0.
The current solution for backward compatibility above does not work since the function starts with an underscore.
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
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 🙃