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

[Tune] Dynamically identify PyTorch Lightning Callback hooks #30045

Merged
merged 6 commits into from
Nov 8, 2022

Conversation

amogkam
Copy link
Contributor

@amogkam amogkam commented Nov 7, 2022

Our current PTL integration Callback hooks are hard coded and contain deprecated hooks which log a warning: #27487.

Some of these hooks have already been deprecated in PTL 1.6, others are deprecated in PTL 1.7, and others in 1.8. Instead of maintaining a different PTL integration callback for each PTL version, we instead dynamically identify the available hooks for whatever PTL version the user has installed.

Closes #27487
Closes https://github.com/anyscale/product/issues/14165

Why are these changes needed?

Related issue number

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/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

davzaman and others added 5 commits August 10, 2022 16:17
…r modifying deprecated functions), reorganized the order to match the rest of the functions, along with the integration tests.

Signed-off-by: Davina Zaman <[email protected]>
Signed-off-by: Amog Kamsetty <[email protected]>
Signed-off-by: Amog Kamsetty <[email protected]>
@Yard1
Copy link
Member

Yard1 commented Nov 7, 2022

Can we test this somehow?

@amogkam
Copy link
Contributor Author

amogkam commented Nov 7, 2022

We already have tests for this @Yard1

@Yard1
Copy link
Member

Yard1 commented Nov 7, 2022

Would it be possible to test the new logic introduced in this PR, though? Eg. use runtime envs to install 2 PTL versions and see that the hooks were detected correctly?

@amogkam
Copy link
Contributor Author

amogkam commented Nov 7, 2022

Hmm but the logic introduced in this pr is already being tested right? There’s no version specific logic introduced in this pr.

Should we be testing all of our integrations against all external library versions?

@Yard1
Copy link
Member

Yard1 commented Nov 7, 2022

I suppose that's true, we don't really have a precedent here. I just find it a bit worrying that the intention of this PR is to make the callback version-independent but we have no way of checking if that was achieved

@amogkam
Copy link
Contributor Author

amogkam commented Nov 7, 2022

I agree, but to be fair, the intention of the callback in general (even before this PR) was also to be version independent.

Ideally, there would be a good way to test this, but we end up having to test against lots of different versions.

@Yard1
Copy link
Member

Yard1 commented Nov 7, 2022

Yeah, that's understandable. The logic looks good to me!

Signed-off-by: Amog Kamsetty <[email protected]>
@amogkam
Copy link
Contributor Author

amogkam commented Nov 8, 2022

Failing test is flaky on master, going to merge

@amogkam amogkam merged commit 60cde11 into ray-project:master Nov 8, 2022
@amogkam amogkam deleted the ptl-1.6-warnings branch November 8, 2022 02:08
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
…ject#30045)

Our current PTL integration Callback hooks are hard coded and contain deprecated hooks which log a warning: ray-project#27487.

Some of these hooks have already been deprecated in PTL 1.6, others are deprecated in PTL 1.7, and others in 1.8. Instead of maintaining a different PTL integration callback for each PTL version, we instead dynamically identify the available hooks for whatever PTL version the user has installed.

Signed-off-by: Davina Zaman <[email protected]>
Signed-off-by: Amog Kamsetty <[email protected]>
Co-authored-by: Davina Zaman <[email protected]>
Signed-off-by: Weichen Xu <[email protected]>
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
6 participants