Skip to content

Add a warning about the combination of gRPC Proxy and Journal Storage#6097

Merged
y0z merged 2 commits intooptuna:masterfrom
toshihikoyanase:add-warning-about-unsupported-backend-for-grpc-proxy
Jun 4, 2025
Merged

Add a warning about the combination of gRPC Proxy and Journal Storage#6097
y0z merged 2 commits intooptuna:masterfrom
toshihikoyanase:add-warning-about-unsupported-backend-for-grpc-proxy

Conversation

@toshihikoyanase
Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase commented May 27, 2025 •
edited
Loading

Motivation

This PR adds a warning to the GrpcStorageProxy class documentation to clarify a known limitation regarding unsupported storage types. The corresponding issue is #6084.

Description of the changes

  • Add a warning section about combining the gRPC proxy and journal storage.

Design choices

  1. I'm unsure if we can add a link to the tracking issue (i.e., JournalStorage fails frequently in distributed optimization setups in combination with GrpcProxyStorage #6084).
  2. IMHO, we can move this warning as well as the warning about SQLite3 to the reference of run_grpc_proxy_server since users specify the storage there. Regarding consistency, I placed this warning in the GrpcStorageProxy reference. The warning should be placed in the server-side document. I fixed the PR in the commit of 2f24020.
  3. We can also note that the cache mechanism of RDBStorage is enabled when it is created via the get_storage function.

@toshihikoyanase toshihikoyanase added the document Documentation related. label May 27, 2025
@toshihikoyanase
Copy link
Copy Markdown
Member Author

toshihikoyanase commented May 27, 2025

We can see the rendering results in https://optuna--6097.org.readthedocs.build/en/6097/reference/generated/optuna.storages.GrpcStorageProxy.html#optuna.storages.GrpcStorageProxy.

image

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented May 28, 2025

@y0z @gen740 @sawa3030 Could you review this PR?

@toshihikoyanase
Copy link
Copy Markdown
Member Author

toshihikoyanase commented May 28, 2025

I thought twice and moved the warning from the client document to the server one, as users specify the storage there.
The corresponding commit is 2f24020.

@codecov
Copy link
Copy Markdown

codecov bot commented May 28, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.27%. Comparing base (e1e30e7) to head (2f24020).
⚠️ Report is 544 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6097      +/-   ##
==========================================
- Coverage   88.35%   88.27%   -0.08%     
==========================================
  Files         207      207              
  Lines       13995    13995              
==========================================
- Hits        12365    12354      -11     
- Misses       1630     1641      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sawa3030
Copy link
Copy Markdown
Collaborator

sawa3030 commented May 30, 2025

The updated documentation looks good to me. By the way, I was wondering if it would be better to add a warning message in run_grpc_proxy_server when the storage type passed to it is JournalStorage.

@toshihikoyanase
Copy link
Copy Markdown
Member Author

toshihikoyanase commented Jun 3, 2025

Thank you very much for your feedback.
Regarding runtime warnings, since an experimental warning for the gPRC Proxy is already in place, we would like to avoid introducing too many additional runtime warnings. For this PR, we may prefer to focus on modifying the reference for now. Would this approach be acceptable to you?

gen740
gen740 approved these changes Jun 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@gen740 gen740 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gen740 gen740 removed their assignment Jun 3, 2025
sawa3030
sawa3030 approved these changes Jun 4, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@sawa3030 sawa3030 left a 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 explanation. LGTM

y0z
y0z approved these changes Jun 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@y0z y0z left a comment

Choose a reason for hiding this comment

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

LGTM

@y0z y0z added this to the v4.4.0 milestone Jun 4, 2025
@y0z y0z merged commit 632256b into optuna:master Jun 4, 2025
16 checks passed
@toshihikoyanase toshihikoyanase deleted the add-warning-about-unsupported-backend-for-grpc-proxy branch June 4, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@y0z y0z y0z approved these changes

@gen740 gen740 gen740 approved these changes

@sawa3030 sawa3030 sawa3030 approved these changes

Assignees

@y0z y0z

Labels

document Documentation related.

Projects

None yet

Milestone

v4.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@toshihikoyanase @HideakiImamura @sawa3030 @y0z @gen740