Skip to content

fix(storage): Fix threading lock logic#5922

Merged
not522 merged 1 commit intooptuna:masterfrom
gen740:fix_journal_storage_thread_lock
Jan 31, 2025
Merged

fix(storage): Fix threading lock logic#5922
not522 merged 1 commit intooptuna:masterfrom
gen740:fix_journal_storage_thread_lock

Conversation

@gen740
Copy link
Copy Markdown
Member

@gen740 gen740 commented Jan 10, 2025

Motivation

As shown in #5862, journal storage sometimes fails on free-threaded Python. This is because the journal store accesses the shared data without getting the lock.

Description of the changes

Increase the lock range in the journal storage.

@github-actions
Copy link
Copy Markdown
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Jan 19, 2025
@gen740 gen740 marked this pull request as ready for review January 24, 2025 07:39
@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Jan 26, 2025
@nabenabe0928 nabenabe0928 added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Jan 27, 2025
@nabenabe0928
Copy link
Copy Markdown
Contributor

@c-bata @not522 Could you review this PR?

Copy link
Copy Markdown
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.

@c-bata c-bata removed their assignment Jan 28, 2025
Copy link
Copy Markdown
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.

The change is LGTM, but I'm not confident it relates to the free-threaded Python. I confirmed that the reported error in #5862 occurs in 3.13.1 and 3.13.1t.
Is it okay to merge this PR as a ordinary bug fix?

@gen740
Copy link
Copy Markdown
Member Author

gen740 commented Jan 31, 2025

@not522
In my environment, the reported error in #5862 does not occur after this change in both 3.13.1 and 3.13.1t. Could you give me more details?

@not522
Copy link
Copy Markdown
Member

not522 commented Jan 31, 2025

Yes, this PR fixes the error in the my both environments. So, the motivation described above is misleading?

As shown in #5862, journal storage sometimes fails on free-threaded Python.

I think this PR is not limited to the free-threaded Python.

@gen740
Copy link
Copy Markdown
Member Author

gen740 commented Jan 31, 2025

I see, that sounds reasonable.

@not522
Copy link
Copy Markdown
Member

not522 commented Jan 31, 2025

Thanks! My question is resolved, so I merge this PR.

@not522 not522 added this to the v4.3.0 milestone Jan 31, 2025
@not522 not522 merged commit d89b3fb into optuna:master Jan 31, 2025
@gen740 gen740 deleted the fix_journal_storage_thread_lock branch July 11, 2025 04:24
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants