-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
updated optuna_search to allow users to configure optuna storage #48547
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ravi Dalal <[email protected]>
Signed-off-by: Ravi Dalal <[email protected]>
Signed-off-by: Ravi Dalal <[email protected]>
Signed-off-by: Ravi Dalal <[email protected]>
@hongpeng-guo @justinvyu @matthewdeng @raulchen @woshiyyya could you please take a look at this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravi-dalal Thanks for contributing to the ray project! Left a few comments. One question that I have is on the study_name argument, please elaborate more why this arg is necessary to be here with the storage.
Signed-off-by: Ravi Dalal <[email protected]>
Signed-off-by: Ravi Dalal <[email protected]>
@hongpeng-guo Thank you for taking time to review this PR. I updated the PR following your comments. PTAL when you get time. |
@hongpeng-guo gentle reminder to take another look at this PR. Thank you! |
@hongpeng-guo @justinvyu @matthewdeng apologies for bugging you again. could you please take a look at this PR? I think it is very close to be ready to get merged, so you may not have to spend much time on it. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for contributing! I think it looks good in general. Could you please add some unit tests before we can merge this PR?
Co-authored-by: Hongpeng Guo <[email protected]> Signed-off-by: Ravi Dalal <[email protected]>
Co-authored-by: matthewdeng <[email protected]> Signed-off-by: Ravi Dalal <[email protected]>
Co-authored-by: matthewdeng <[email protected]> Signed-off-by: Ravi Dalal <[email protected]>
@hongpeng-guo is |
Yes, it is the correct place to test optuna searchers. Basically, we want to show that when storage are configured, the |
Signed-off-by: Ravi Dalal <[email protected]>
Thanks for confirmation, @hongpeng-guo . I added |
Why are these changes needed?
The OptunaSearch currently uses in-memory storage and does not provide a way to configure any other storages (e.g. database). This update add a configurable parameter to OptunaSearch that can be set to a valid Optuna Storage.
Related issue number
Closes #48500
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.