Conversation
|
After fixing some bugs, I updated the microbenchmark. |
sile
left a comment
There was a problem hiding this comment.
Thank you for the PR! I took the first look and left some review comments.
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
+ Coverage 90.50% 91.13% +0.62%
==========================================
Files 131 144 +13
Lines 11396 12628 +1232
==========================================
+ Hits 10314 11508 +1194
- Misses 1082 1120 +38
Continue to review full report at Codecov.
|
sile
left a comment
There was a problem hiding this comment.
Thank you for addressing my review comments. This PR seems almost okay but I left a comment to follow the storage specification change.
optuna/storages/cached_storage.py
Outdated
| param_value_internal | ||
| ) | ||
| cached_trial.distributions[param_name] = distribution | ||
| if cached_dist: |
There was a problem hiding this comment.
I guess the line
self._dirty_trials.add(trial_id)
should be moved under if cached_dist: .
The logic here is a bit difficult too. Some comments, like "If cached_dist isn't available, we have to access the storage in order to check distribution compatibility." would help readers.
There was a problem hiding this comment.
Thank you for your comment. Indeed, the line should be moved, and the whole logic requires more readability. I'll address them.
|
This PR should not be merged before #1191. |
28008cc to
9ba2cf6
Compare
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
+ Coverage 86.61% 87.01% +0.40%
==========================================
Files 93 94 +1
Lines 6924 7233 +309
==========================================
+ Hits 5997 6294 +297
- Misses 927 939 +12
Continue to review full report at Codecov.
|
|
I rearranged commits and now this PR passed the updated tests #1191. Some implementation has been changed from the latest reviews:
@sile @toshihikoyanase Please review this PR when you have time. |
sile
left a comment
There was a problem hiding this comment.
@ytsmiling Thanks! Now this RP seems great but I left a few very minor comments.
Co-authored-by: Takeru Ohta <phjgt308@gmail.com>
sile
left a comment
There was a problem hiding this comment.
LGTM! Thank you for your swift fix.
There was a problem hiding this comment.
Thank you for your great PR. I'm in the middle of the review of logic, but let me share some comments about styles.
This is just a comment, but I noticed two things:
FrozenTrialis notfrozenanymore. The attribute values are updated to keep the cache up-to-date. Not now, but we may rename it in the future.- This PR directly manipulates SQLAlchemy queries while the previous implementation tried to capslate them in
optuna/rdb/models.py. It may be inevitable to speed up storages.
toshihikoyanase
left a comment
There was a problem hiding this comment.
The changes regarding the logic seem good to me.
I'll approve the PR after you check my comments about styles.
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
|
@toshihikoyanase Thank you for reviewing this PR.
Actually, the mutation of
Yes, I think we should remove methods in the |
toshihikoyanase
left a comment
There was a problem hiding this comment.
Thank you for your swift actions. LGTM!
Motivation
Flushing updates of trials to persistent storages on every update (including parameter suggests) can be a significant bottleneck. This PR mitigates the bottleneck by introducing a wrapper class that wraps the `RDBStorage/ class and providing the lazy-sync capability.
Description of the changes
CachedStorage, which is a wrapper ofRDBStorageand provide the lazy-sync capability.CachedStorageonly supportsRDBStorage, it is straightforward to support other classes such asRedisStorage. Thus,CachedStoragefile is placed directly under theoptuna/storagesclass.RDBStorage.FrozenTrialwhen creating a new trial. Note,create_new_trialinBaseStorageonly returns the trial-id.FrozenTrialand flush the updates to persistent storages.Current Status
CachedStorageclass.Microbench
(DB: official mysql docker image (tag:8.0.19), storage: SSD, CPU: Intel Core i7-6700K, python: 3.5.2)
This PR
Current master
Benchmark code: