-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix broken pytorch-lightning import #664
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #664 +/- ##
==========================================
- Coverage 88.30% 88.22% -0.09%
==========================================
Files 43 43
Lines 3139 3142 +3
Branches 279 279
==========================================
Hits 2772 2772
- Misses 325 328 +3
Partials 42 42
☔ View full report in Codecov by Sentry. |
@@ -22,7 +22,11 @@ | |||
from pytorch_lightning.callbacks.model_checkpoint import ModelCheckpoint | |||
from pytorch_lightning.loggers.logger import Logger, rank_zero_experiment | |||
from pytorch_lightning.utilities import rank_zero_only | |||
from pytorch_lightning.utilities.logger import _scan_checkpoints | |||
|
|||
try: |
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.
We could also do an explicit version check but I guess it is equally "ugly
if pkg.parse_version(version('pytorch_lightning')) < pkg.parse_version('2.0'):
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 the quick fix, @dberenbaum. @daavoo Guess I would agree with the sentiment of the meme, except that you guys are importing a private member ( |
That is true for this case but the rest of the import patches are for public things, including the pure package name (i.e. Lightning-AI/pytorch-lightning#16688). |
Oh, yes, things are definitely not in a great intermediate state on their end. BTW I meant to put a smily after my comment. I don't have problem with how things are and think it makes sense to limit the effort put into this as long as it works. And if it doesn't, you guys are swift in catching up! |
No worries! I didn't get it as not friendly and I was also not trying to be defensive, async communication is what it is 😅 |
Per #625 (comment).
The current import works for
pytorch-lightning<2.0
. Added a nested try/except to work forpytorch-lightning>2.0
. It's getting a bit ugly, but don't see a good reason not to support as many versions as possible for now.