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

Optuna resume study #2647

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

michelkok
Copy link

@michelkok michelkok commented Apr 21, 2023

Motivation

When using the Optuna sweeper and the storage option, the optuna.study will actually be resumed (ref).
However, the sweeper will reset the job_idx to 0 and the n_trials_to_go to the number originally configured (i.e., before interrupting the sweep).

It's confusing that the reloading is done but the configured job parameters are not updated. This PR makes it explicit by allowing the configuration of the load_if_exists parameter. If True (by default - for strongest backwards compatibility) the study is loaded if it already exists. The n_trials_to_go and job_idx are then updated accordingly. If False, an Optuna error will be raised. Note this parameter acts exactly in accordance with optuna.

Have you read the Contributing Guidelines on pull requests?

Yes, but it's my first contribution so any feedback is welcome.

Test Plan

I added an additional test to demonstrate and test the desired behavior.

Related Issues and PRs

This is related to #1407 and should close #1679. (W.r.t. #1679 the garbage collecting can be done in a callback, I can optionally demonstrate in that issue before it's closed).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 21, 2023
@michelkok
Copy link
Author

Could somebody help me understand the errors? I am not sure how to fix my code in order to have it pass the tests.

@odelalleau
Copy link
Collaborator

There's an annoying issue that hasn't been fixed yet in master that's adding noise to the tests. Can you include the fix from #2731 in your PR, which hopefully will clean up the CI logs?

@michelkok
Copy link
Author

Thanks, it doesn't seem to help though.

@odelalleau
Copy link
Collaborator

Hmm, yeah there are a bunch of errors that seem unlikely to have anything to do with your PR. I'm afraid CI is currently broken.

@michelkok
Copy link
Author

Okay, thanks for confirming. I'll hope someone will review and accept this anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Resume study and avoid OOM when optimizing with Optuna plugin
3 participants