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

fix broken pytorch-lightning import #664

Merged
merged 1 commit into from
Aug 16, 2023
Merged

fix broken pytorch-lightning import #664

merged 1 commit into from
Aug 16, 2023

Conversation

dberenbaum
Copy link
Collaborator

Per #625 (comment).

The current import works for pytorch-lightning<2.0. Added a nested try/except to work for pytorch-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.

@dberenbaum dberenbaum requested a review from daavoo August 15, 2023 15:53
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (bdf39da) 88.30% compared to head (4de9f22) 88.22%.
Report is 1 commits behind head on main.

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              
Files Changed Coverage Δ
src/dvclive/lightning.py 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -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:
Copy link
Contributor

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'):

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daavoo daavoo merged commit 1aa3e05 into main Aug 16, 2023
@daavoo daavoo deleted the fix-lightning-import branch August 16, 2023 09:25
@daavoo daavoo added A: frameworks Area: ML Framework integration bugfix labels Aug 16, 2023
@aschuh-hf
Copy link

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 (_scan_checkpoints) that clearly isn't part of their public API...

@daavoo
Copy link
Contributor

daavoo commented Aug 17, 2023

@daavoo Guess I would agree with the sentiment of the meme, except that you guys are importing a private member (_scan_checkpoints) that clearly isn't part of their public API...

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).
Anyhow the main thing we have been doing "wrong" is not moving the logger into the framework repo, although it is in the plans.

@aschuh-hf
Copy link

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!

@daavoo
Copy link
Contributor

daavoo commented Aug 17, 2023

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 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: frameworks Area: ML Framework integration bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants