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

Update TuneCallback for Pytorch-Lightning 1.6+ #27766

Merged
merged 0 commits into from
Aug 10, 2022

Conversation

davzaman
Copy link
Contributor

@davzaman davzaman commented Aug 10, 2022

Why are these changes needed?

Getting a ton of warnings from pytorch-lightning about deprecation of certain hooks. The TuneCallback has old hooks such as keyboard_interrupt which have been deprecated. I updated the callback to the new hooks and updated the integration tests.

The deprecated hooks and their new versions (In pl v1.6+) are:

  • on_keyboard_interrupt -> on_exception
  • on_init_start -> just deprecated no new version
  • on_init_end -> just deprecated no new version
  • on_batch_start -> on_train_batch_start
  • on_batch_end -> on_train_batch_end
  • on_epoch_end -> on_<train/validation/test>_epoch_end
  • on_epoch_start -> on_<train/validation/test>_epoch_start

Related issue number

Closes #27487

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/

I spent a few hours trying to wrangle my environment. Unfortunately I'm on windows using WSL and have an old version of Debian for some reason which only supports gcc-6 (not even close to gcc-9 which is required for building the project so I can run the tests). Given this roadblock and the fact that the fixes were minimal and simple I decided to push out the changes.

  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for opening this PR! Let's wait for tests to pass and I'd be happy to help merge!

@davzaman
Copy link
Contributor Author

I think I may have caused a mess trying to go back and sign-off my commits, let me see how I can fix all these extra commits.

@davzaman davzaman merged commit 54a9b1d into ray-project:master Aug 10, 2022
@davzaman
Copy link
Contributor Author

davzaman commented Aug 10, 2022

I have 0 idea how this got forced to merge? I was trying to clean up my commit, I am so sorry. Please let me know how I can fix this!

It looks like the changes were accepted while I was making this adjustment so nothing got merged.

The contributing docs on the documentation site do not mention signoffs by the way!

@richardliaw
Copy link
Contributor

Hmm, seems like you merged into your own master? It definitely didn't affect the ray project master so don't worry :)

@davzaman
Copy link
Contributor Author

Would it be possible to delete this PR and try again? I cleaned up the commit so it should not fail the bot signoff check.

@richardliaw
Copy link
Contributor

yep, think you don't need to do anything here.

@davzaman
Copy link
Contributor Author

Thanks for coordinating with me! I resubmitted at #27770

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.

[Tune: Python] Pytorch-lightning deprecated hooks in TuneReportCallback
2 participants