- Notifications
You must be signed in to change notification settings - Fork1.1k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Introduce client-side cache in GrpcStorageProxy#5872
Conversation
Co-authored-by: mamu <mamu@preferred.jp>
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 for the follow-up. Looks very good. i confirmed that all logics are consistent with the cache logic of_CachedStorage and match with the current implementation.
Sorry for the confusion. It seems that we need to change not onlyget_all_trials but also other methods of storages, such thatdelete_study.
| @@ -102,6 +105,7 @@ def delete_study(self, study_id: int) -> None: | |||
| if e.code() == grpc.StatusCode.NOT_FOUND: | |||
| raise KeyError from e | |||
| raise | |||
| self._cache.delete_study_cache(study_id) | |||
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.
I've found an issue regarding the cache invalidation.
With the current schema of Optuna's SQLite3 database, SQLite3 reusesstudy_id, as shown below.
sqlite> .schema studiesCREATETABLEstudies ( study_idINTEGERNOT NULL, study_nameVARCHAR(512)NOT NULL,PRIMARY KEY (study_id));
>>>storage=optuna.storages.RDBStorage("sqlite:///foo.db")>>>storage.create_new_study(directions=[optuna.study.StudyDirection.MINIMIZE])[I2024-12-2017:10:54,500]AnewstudycreatedinRDBwithname:no-name-2a8665b6-fb78-48d5-937b-cd2c5a71f9ac1>>>storage.delete_study(study_id=1)>>>storage.create_new_study(directions=[optuna.study.StudyDirection.MINIMIZE])[I2024-12-2017:11:04,641]AnewstudycreatedinRDBwithname:no-name-0b71c4ee-123f-4894-9903-4014c6581942
Consequently, when multiple gRPC storage proxy servers are running andstorage.delete_study() is called, the cache may not be invalidated. This issue does not occur with MySQL, as shown below.
>>>importoptuna>>>storage=optuna.storages.get_storage("mysql+pymysql://optuna:password@127.0.0.1:3306/optuna")>>>storage.create_new_study(directions=[optuna.study.StudyDirection.MINIMIZE])[I2024-12-2018:37:24,719]AnewstudycreatedinRDBwithname:no-name-95110374-00cc-4136-a1a3-58d8e5e88cba132>>>storage.delete_study(study_id=132)>>>storage.create_new_study(directions=[optuna.study.StudyDirection.MINIMIZE])[I2024-12-2018:37:30,603]AnewstudycreatedinRDBwithname:no-name-feac51b7-be94-407c-8b83-2e0e8e2d0b9d133
This issue is relatively limited, as it only arises when multiple gRPC storage proxies are running with a single SQLite3 database, and optuna.delete_study() are executed. So, my suggestion is to proceed with this PR and address this issue in the future by addingAUTOINCREMENT (please refer tohttps://www.sqlite.org/autoinc.html for more details).
866b980 toe9e24c5Compare@y0z@nabenabe0928 Could you review 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.
I feel like the following methods can also use cache, but do you wanna include them in this PR, make another PR, or do not wanna do it at all?
- get_study_directions
- get_trial_id_from_study_id_trial_number
- get_trial
- get_study_name_from_id
- get_study_id_from_name
optuna/storages/_grpc/client.py Outdated
| @@ -53,6 +54,9 @@ class GrpcStorageProxy(BaseStorage): | |||
| host: The hostname of the gRPC server. | |||
| port: The port of the gRPC server. | |||
| .. warning:: | |||
| Currently, gRPC storage proxy does not work as expected when using a SQLite3 database as the backend and calling :func:`optuna.delete_study`. | |||
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.
Did you mean this?
I understood the SQLite3 part, but I am not sure about thedelete_study part.
Could you please confirm?
| Currently,gRPCstorageproxydoesnotworkasexpectedwhenusingaSQLite3databaseasthebackendandcalling :func:`optuna.delete_study`. | |
| gRPCstorageproxydoesnotaccept :func:`optuna.delete_study`whenthebackendisSQLite3database. |
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.
The gRPC storage proxy does acceptoptuna.delete_study() without raising exceptions. However, it may result in an inconsistent state when using an SQLite3 database as the backend and multiple storage proxies are running.
| int64 study_id = 1; | ||
| repeated TrialState states = 2; | ||
| repeated int64 included_trial_ids = 2; |
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.
Probably,including is better if I understand the context of this request.
| repeated int64included_trial_ids = 2; | |
| repeated int64including_trial_ids = 2; |
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.
I think the current naming and implementation are consistent with theRDBStorage (ref).
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.
Thank you for your review! I'll address this with RDBStorage's_get_trials() method in the following pull request, as this is a private api.
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.
I will approve this PR once I confirm the performance.
The changes look good to me. |
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.
Hmmm, I tried with Plus, it took me so long to finish I used MySQL as a database and Python 3.11 on Ubuntu 20.04. |
I measured the runtime of pure RDBStorage in my env and it took me about a minute for the code below. Benchmark Codeimportosimporttimeimportoptunadefobjective(trial:optuna.Trial)->float:s=0.0foriinrange(10):s+=trial.suggest_float(f"x{i}",-10,10)**2returnsmysql_url=os.environ["OPTUNA_STORAGE"]study=optuna.create_study(sampler=optuna.samplers.RandomSampler(),storage=mysql_url)start=time.time()study.optimize(objective,n_trials=1000,n_jobs=10)print(time.time()-start) |
I confirmed that this PR is faster than the current master by using the following benchmark code. It looks like the
from __future__importannotationsimportosfromoptuna.storagesimportrun_grpc_proxy_serverimportoptunaimportsignalSTORAGE_URL="YOUR_MYSQL_URL"defhandler(signum,frame):print("Received signal",signum)exit(0)if__name__=="__main__":signal.signal(signal.SIGINT,handler)signal.signal(signal.SIGTERM,handler)run_grpc_proxy_server(optuna.storages.get_storage(STORAGE_URL),host="localhost",port=13000, )
from __future__importannotationsimportargparsefromconcurrent.futuresimportProcessPoolExecutor,as_completedfromcollections.abcimportSequencefromcollections.abcimportContainerimportcProfileimporttimeimportnumpyasnpimportoptunaimportthreadingfromtypingimportLiteralfromoptuna.storagesimportGrpcStorageProxy,BaseStorageimportosimportuuidfromtimeimportsleepstudy_name="grpc-example"n_workers=10n_repeat=5n_trials=1000STORAGE_KIND=Literal["grpc","rdb","middleware"]STORAGE_URL="YOUR_MYSQL_URL"defget_storage(storage_kind:STORAGE_KIND)->BaseStorage:ifstorage_kind=="grpc":returnGrpcStorageProxy(host="localhost",port=13000)elifstorage_kind=="rdb":returnoptuna.storages.get_storage(STORAGE_URL)defobjective(trial:optuna.Trial)->float:s=0.0foriinrange(10):trial.set_user_attr(f"attr{i}","attr value")foriinrange(10):s+=trial.suggest_float(f"x{i}",-10,10)**2returnsdefstudy_optimize(storage_kind:STORAGE_KIND,n_trials:int)->int:start=time.time()storage=get_storage(storage_kind)whileTrue:try:study=optuna.load_study(storage=storage,study_name=study_name,sampler=optuna.samplers.RandomSampler())breakexcept:sleep(10)study.optimize(objective,callbacks=[optuna.study.MaxTrialsCallback(n_trials)])elapsed=time.time()-startreturnelapseddefbench_study_optimize(storage_kind:STORAGE_KIND)->list[int]:elapsed= []study=optuna.create_study(storage=get_storage("rdb"),study_name=study_name,load_if_exists=True,sampler=optuna.samplers.RandomSampler(), )study_optimize(storage_kind,n_trials)returnelapseddefmain(storage:STORAGE_KIND,benchmark_file:str):try:optuna.delete_study(storage=get_storage("rdb"),study_name=study_name)except:passcProfile.runctx(f"bench_study_optimize('{storage}')",globals(),locals(),filename=benchmark_file, )optuna.delete_study(storage=get_storage(storage),study_name=study_name)if__name__=="__main__":parser=argparse.ArgumentParser()parser.add_argument("--storage",type=str,required=True)parser.add_argument("--benchmark_file",type=str,required=True)args=parser.parse_args()main(args.storage,args.benchmark_file)
|
This pull request has not seen any recent activity. |
Thank you for your review. I think we don't need to use cache for these methods unless they become a bottleneck. What do you think? |
Let me unassign@y0z and assign@HideakiImamura instead. |
Ok, I figured out why I did not get the speedup. I originally used the following code, leading to the use of RDBStorage without cache in local process: importosfromoptuna.storagesimportrun_grpc_proxy_serverfromoptuna.storagesimportRDBStoragestorage=RDBStorage(os.environ["OPTUNA_STORAGE"])run_grpc_proxy_server(storage,host="localhost",port=13000) However, when I used importosfromoptuna.storagesimportrun_grpc_proxy_serverfromoptuna.storagesimportget_storagestorage=get_storage(os.environ["OPTUNA_STORAGE"])run_grpc_proxy_server(storage,host="localhost",port=13000) Master: 112 seconds, This PR: 61 seconds. Probably, the doc-string should be changedhere. |
Followups:
|
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.
LGTM.
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.
Thank you for the update, LGTM!
Follow-up#5872: Update the docstring of `run_grpc_proxy_server`
Motivation
Follow-up#5852
To make
GrpcStorageProxyfaster by introducing a client-side cache on GrpcStorageProxy.Description of the changes
This PR makes
GrpcStorageProxyfaster by cachestorage.get_all_trialscalls. Here is the performance comparison results and the benchmark script I used.masterbranch