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

Fix JSONDecodeError in JournalStorage #5195

Merged
merged 12 commits into from Feb 5, 2024

Conversation

y0z
Copy link
Member

@y0z y0z commented Jan 22, 2024

Motivation

Description of the changes

  • Maintain remaining_log_size to allow writing by another process while reading the log.

@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Jan 22, 2024
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (45a9dc9) 89.41% compared to head (33fc7e9) 89.35%.
Report is 38 commits behind head on master.

Files Patch % Lines
optuna/storages/_journal/file.py 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5195      +/-   ##
==========================================
- Coverage   89.41%   89.35%   -0.07%     
==========================================
  Files         206      206              
  Lines       15175    15106      -69     
==========================================
- Hits        13569    13498      -71     
- Misses       1606     1608       +2     

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

@y0z y0z changed the title Fix JSONDecodeError in JournalStorage #5138 Fix JSONDecodeError in JournalStorage Jan 22, 2024
@ernestum
Copy link

Hi thanks for fixing this. I just did a test run of this branch on the cluster with 100 parallel workers and observed no more crashes :-)

Copy link
Member

@c-bata c-bata 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 your pull request. I left two comments.

optuna/storages/_journal/file.py Outdated Show resolved Hide resolved
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but a second thorough review would be helpful.

@c-bata c-bata added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Jan 23, 2024
@c-bata c-bata removed their assignment Jan 23, 2024
@c-bata c-bata self-assigned this Jan 23, 2024
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the update!

@c-bata c-bata removed their assignment Jan 26, 2024
Co-authored-by: Naoto Mizuno <gobou522@gmail.com>
@y0z
Copy link
Member Author

y0z commented Jan 31, 2024

I applied all the comments. PTAL.

@not522
Copy link
Member

not522 commented Feb 1, 2024

Sorry, I realize that my previous comment (using os.linesep) doesn't work on Windows. It is because the line separator for what_to_write.encode("utf-8") is \n instead of \r\n. Could you revert the last commit?

@y0z
Copy link
Member Author

y0z commented Feb 1, 2024

I see. I've reverted the change.

Copy link
Member

@not522 not522 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 update. LGTM.

@not522 not522 merged commit b2350a8 into optuna:master Feb 5, 2024
21 of 22 checks passed
@not522 not522 changed the title Fix JSONDecodeError in JournalStorage Fix JSONDecodeError in JournalStorage Feb 5, 2024
@not522 not522 added this to the v3.6.0 milestone Feb 5, 2024
@y0z y0z deleted the feature/fix_journalstorage branch February 5, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants