Skip to content

Change caching implementation of MOTPE#2406

Merged
Crissman merged 10 commits intooptuna:masterfrom
y0z:fix_motpe_cache
Mar 31, 2021
Merged

Change caching implementation of MOTPE#2406
Crissman merged 10 commits intooptuna:masterfrom
y0z:fix_motpe_cache

Conversation

@y0z
Copy link
Copy Markdown
Member

@y0z y0z commented Feb 27, 2021

Motivation

This PR fixes issue #2331.

Description of the changes

This PR changes the caching implementation of MOTPE.

Previously, MOTPE uses trial.system_attrs to cache its internal parameters. This causes optuna.exceptions.StorageInternalError after hundreds of iterations because of exceeding max length when the cache becomes large.

Now, MOTPE uses its member variables to cache its internal parameters and this fixes the issue mentioned above.

@github-actions github-actions Bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Feb 27, 2021
@g-votte g-votte added the sprint-20210227 PR from the online sprint event Feb 27, 2021. label Feb 27, 2021
@HideakiImamura HideakiImamura added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. and removed enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. labels Feb 27, 2021
@keisuke-umezawa
Copy link
Copy Markdown
Member

keisuke-umezawa commented Feb 28, 2021 •
edited
Loading

@HideakiImamura @c-bata
If you have time, could you review it?

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Mar 1, 2021

@y0z I think the CI will pass if you merge the master branch. Could you merge the master branch?

HideakiImamura
HideakiImamura reviewed Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Left some comments. PTAL.

Comment thread optuna/samplers/_tpe/multi_objective_sampler.py Outdated
Comment thread optuna/samplers/_tpe/multi_objective_sampler.py Outdated
HideakiImamura
HideakiImamura reviewed Mar 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I have another suggestions. PTAL.

Comment thread optuna/samplers/_tpe/multi_objective_sampler.py Outdated
Comment thread optuna/samplers/_tpe/multi_objective_sampler.py Outdated
@y0z
Copy link
Copy Markdown
Member Author

y0z commented Mar 11, 2021

In ci/circleci: document, urllib.error.HTTPError: HTTP Error 503: Service Unavailable has occured though after merging #2459.

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Mar 12, 2021

@y0z Thanks for your swift actions. The CI failure has been fixed now by #2468. Could you merge the master branch again?

HideakiImamura
HideakiImamura approved these changes Mar 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

LGTM!

@c-bata Please merge this PR after your reviews.

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Mar 16, 2021

@c-bata: @Crissman would like to review the harder PRs, so let me re-assign this PR to him.

@HideakiImamura HideakiImamura assigned Crissman and unassigned c-bata Mar 16, 2021
@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Mar 16, 2021

@Crissman Could you review this PR if you have time? It would be great if you could double-check if the problem reported in #2331 has been resolved.

@Crissman
Copy link
Copy Markdown
Contributor

Crissman commented Mar 23, 2021

This issue appears fixed by #2395 . Please confirm and close this PR and original issue in #2331. 🙇‍♂️

@y0z
Copy link
Copy Markdown
Member Author

y0z commented Mar 23, 2021 •
edited
Loading

I think that the memory-based cache (this PR) is more preferable to the DB-based cache (current implementation) because users generally do not need the cache data for MOTPE's internal implementation in their DB.
In #2331, the author said

I actually do not need this information myself, so maybe there is an option to not write it to the RDB.

Therefore, this PR is still useful and valuable.

@HideakiImamura HideakiImamura added this to the v2.7.0 milestone Mar 29, 2021
@Crissman
Copy link
Copy Markdown
Contributor

Crissman commented Mar 31, 2021

@y0z Thanks for the PR. Discussed with the team, and we agree that this PR is still useful and valuable.

I've read over the code and tested it locally and it runs fine.

I'm not sure the additional test is required. I feel that if the cache was not working, we would see that result in the other multi-objective tests, so I think we can remove this test. What do you think?

@Crissman Crissman added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. and removed bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. labels Mar 31, 2021
@Crissman
Copy link
Copy Markdown
Contributor

Crissman commented Mar 31, 2021 •
edited
Loading

After discussion with @HideakiImamura, agreed test worth doing. Merging!

@Crissman Crissman merged commit 95038eb into optuna:master Mar 31, 2021
@y0z y0z deleted the fix_motpe_cache branch March 31, 2021 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HideakiImamura HideakiImamura HideakiImamura approved these changes

+1 more reviewer

@Crissman Crissman Crissman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Crissman Crissman

@HideakiImamura HideakiImamura

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. sprint-20210227 PR from the online sprint event Feb 27, 2021.

Projects

None yet

Milestone

v2.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@y0z @keisuke-umezawa @HideakiImamura @Crissman @g-votte @c-bata