Fix _pop_waiting_trial_id for finished trial#6012
Conversation
ceabdc9 to
a44f74b
Compare
ff08402 to
7822f24
Compare
|
I've added a test and this PR is now review ready. |
|
@sawa3030 @nabenabe0928 @gen740 Could you review this PR? |
optuna/study/study.py
Outdated
| # The trial is already running. | ||
| continue | ||
| except exceptions.UpdateFinishedTrialError: | ||
| # The trial is already finished. |
There was a problem hiding this comment.
Could you elaborate the note here a bit more?
| assert len(trial_id_set) == num_enqueued | ||
|
|
||
|
|
||
| def test_pop_waiting_trial_id_race_condition() -> None: |
There was a problem hiding this comment.
Confirmed that this test fails with the latest master, but it passes with the modification in this PR.
There was a problem hiding this comment.
Confirmation complete on my end!
nabenabe0928
left a comment
There was a problem hiding this comment.
Almost LGTM!
I left a minor comment, so PTAL!
|
Other than the issue @nabenabe0928 pointed out above, this looks good to me. |
d8677dc to
09af760
Compare
|
Thank you for your reviews. I've updated the comment. PTAL. |
nabenabe0928
left a comment
There was a problem hiding this comment.
The change looks good to me:)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6012 +/- ##
==========================================
- Coverage 88.46% 88.44% -0.03%
==========================================
Files 206 206
Lines 13857 13876 +19
==========================================
+ Hits 12259 12272 +13
- Misses 1598 1604 +6 ☔ View full report in Codecov by Sentry. |
|
@y0z Could you review this PR? |
Motivation
If
_pop_waiting_trial_idtries to change the state for a finished trial, it unexpectedly raisesUpdateFinishedTrialError. This PR changes the implementation to catch the exception.Description of the changes