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.

Sign up for GitHub

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

Jump to bottom

[Tune] Add support for nested hyperparams in PB2 #31502

Merged
merged 12 commits into from
Apr 13, 2023
Merged

[Tune] Add support for nested hyperparams in PB2 #31502

merged 12 commits into from
Apr 13, 2023

Conversation

justinvyu
Copy link
Contributor

Why are these changes needed?

This PR enables nested passing hyperparameters for the PB2 scheduler.

This PR also makes a few minor improvements to PB2 (happy to separate out these changes if needed):

  1. Hyperparameter initialization (if missing from param space) should be sampled uniformly between bounds. Currently, PB2 falls back to PBT for sampling initial hyperparameters, which will just choose between the low/high values.
  2. Allow custom_explore_fn to be passed into PB2 to match PBT functionality. This solves a user request here: https://discuss.ray.io/t/pb2-hyper-parameters-as-integers/8822.

Related issue number

Closes #30124
Closes #29786

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 :(

@justinvyu
Add basic pb2 unit test
3da9600 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add support for nested hyperparams with pb2
5c2c73c 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix hyperparam init for pb2
f859f99 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix bounds validation
4e737d1 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Remove unnecessary import
7bd5c1e 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add support for custom_explore_fn
3708c55 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@stale
Copy link

stale bot commented Feb 11, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 11, 2023
@Yard1
Copy link
Member

Yard1 commented Feb 11, 2023

@justinvyu

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 11, 2023
@stale
Copy link

stale bot commented Mar 14, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 14, 2023
krfricke
krfricke reviewed Mar 31, 2023
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Logic looks good to me, should we aim to ship this PR soon?

python/ray/tune/tests/test_trial_scheduler_pbt.py Outdated
@@ -778,7 +783,162 @@ def test_config(
)


if __name__ == "__main__":
import pytest
def create_pb2_scheduler(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make these functions internal?

And maybe add typehints

@stale stale bot removed stale The issue is stale. It will be closed within 7 days unless there are further conversation labels Mar 31, 2023
@justinvyu
Copy link
Contributor Author

Will aim to get it wrapped up this week!

@justinvyu justinvyu added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Apr 11, 2023
@justinvyu justinvyu requested a review from krfricke April 11, 2023 21:48
krfricke
krfricke approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Thanks!

@krfricke krfricke merged commit 4f2c770 into ray-project:master Apr 13, 2023
vitsai pushed a commit to vitsai/ray that referenced this pull request Apr 17, 2023
@justinvyu @vitsai
[Tune] Add support for nested hyperparams in PB2 (ray-project#31502)
4f2ac07 
This PR enables nested passing hyperparameters for the PB2 scheduler.

This PR also makes a few minor improvements to PB2 (happy to separate out these changes if needed):
1. Hyperparameter initialization (if missing from param space) should be sampled uniformly between bounds. Currently, PB2 falls back to PBT for sampling initial hyperparameters, which will just choose between the low/high values.
2. Allow `custom_explore_fn` to be passed into PB2 to match PBT functionality. This solves a user request here: https://discuss.ray.io/t/pb2-hyper-parameters-as-integers/8822.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@justinvyu @elliottower
[Tune] Add support for nested hyperparams in PB2 (ray-project#31502)
22aa4b9 
This PR enables nested passing hyperparameters for the PB2 scheduler.

This PR also makes a few minor improvements to PB2 (happy to separate out these changes if needed):
1. Hyperparameter initialization (if missing from param space) should be sampled uniformly between bounds. Currently, PB2 falls back to PBT for sampling initial hyperparameters, which will just choose between the low/high values.
2. Allow `custom_explore_fn` to be passed into PB2 to match PBT functionality. This solves a user request here: https://discuss.ray.io/t/pb2-hyper-parameters-as-integers/8822.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@justinvyu @ProjectsByJackHe
[Tune] Add support for nested hyperparams in PB2 (ray-project#31502)
be2c38b 
This PR enables nested passing hyperparameters for the PB2 scheduler.

This PR also makes a few minor improvements to PB2 (happy to separate out these changes if needed):
1. Hyperparameter initialization (if missing from param space) should be sampled uniformly between bounds. Currently, PB2 falls back to PBT for sampling initial hyperparameters, which will just choose between the low/high values.
2. Allow `custom_explore_fn` to be passed into PB2 to match PBT functionality. This solves a user request here: https://discuss.ray.io/t/pb2-hyper-parameters-as-integers/8822.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Jack He <jackhe2345@gmail.com>
@justinvyu justinvyu deleted the pb2_unit_tests branch August 10, 2023 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yard1 Yard1 Yard1 left review comments

@krfricke krfricke krfricke approved these changes

@xwjiang2010 xwjiang2010 Awaiting requested review from xwjiang2010

Assignees

@Yard1 Yard1

@krfricke krfricke

@xwjiang2010 xwjiang2010

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tune] Make PB2 support nested search spaces with hyperparam_bounds [Tune] PB2 is missing unit tests
4 participants
@justinvyu @Yard1 @krfricke @xwjiang2010