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

Upgrade to pytorch-lightning 2 #230

Closed
wants to merge 1 commit into from

Conversation

AmirAlavi
Copy link

This migrates the code to the latest major version of pytorch-lightning. I tried to do it with minimal changes to stay true to the original design philosophy of Casanovo. Feedback is much appreciated.

Summary of changes (I followed this guide to upgrade):

  • LightningLite is deprecated
    • Switch to Fabric
  • auto_select_gpus parameter for Trainer is deprecated
    • Use find_usable_cuda_devices in _get_devices instead. Also, devices can't be 0, so now _get_devices falls back to auto
  • Relying on passed in outputs in on_predict_epoch_end is deprecated
    • Use self.trainer.predict_loop.predictions instead
  • Passing a dictionary to LightingModule.log is deprecated
    • Log as scalar with the "mode" (e.g. "train" or "valid") embedded in string key to the same effect
  • strategy parameter for Trainer cannot be None
    • Return "auto" if no cuda devices

@wfondrie
Copy link
Collaborator

wfondrie commented Aug 7, 2023

Hi @AmirAlavi - thank you for your PR, but we've actually already begun this migration and it will be present in the next release. You can check out the changes in #176.

Casanovo uses a Git Flow-type of system, where our in-development work goes into the dev branch before being migrated into main. There's a lot of other updates waiting to be merged into main too!

@wfondrie wfondrie closed this Aug 7, 2023
@AmirAlavi
Copy link
Author

Ah got it, thanks @wfondrie, don't know how I missed that PR. Thanks! Will stay tuned.

@wfondrie
Copy link
Collaborator

wfondrie commented Aug 7, 2023

No worries - we need to update our contribution docs to be more accurate since the change. Sorry for the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants