-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Prune deprecated utils modules #7503
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
Changes from all commits
7e9c5aa
0ce64af
9769afa
e5ef30c
b9285e4
dee8274
ab97834
f0cbe0c
3f727b4
cfbf72e
b36c5c8
92004ca
578d95e
4869eb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,6 @@ | |
|
|
||
| rank_zero_deprecation("`argparse_utils` package has been renamed to `argparse` since v1.2 and will be removed in v1.4") | ||
|
|
||
| from pytorch_lightning.utilities.argparse import * # noqa: F403 E402 F401 | ||
|
|
||
| # for backward compatibility with old checkpoints (versions < 1.2.0) | ||
| # that need to be able to unpickle the function from the checkpoint | ||
| from pytorch_lightning.utilities.argparse import _gpus_arg_default # noqa: E402 F401 # isort: skip | ||
|
Comment on lines
-5
to
7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @carmocca In theory this should be fine because we still have the import below here for BC.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is that the deprecation tests have been removed but not the deprecation message And if we do, this will be forgotten for eternity haha Is that okay?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the best way would be to add a test for legacy checkpoint which covers this use-case
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, I will never forget the pain it has caused me. 🤣 If you want, I can generate one of these checkpoints and we make a tests that loads it? If someone removes the line, we make sure the test breaks?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, no need for that. If spending time on it, it should be on how to figure out the monkey patching
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, can you prepare a model which would cover those issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all we need to do is add
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can regenerate just some "recent" checkpoints... |
||
This file was deleted.
This file was deleted.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.