Refactor warning handling of _tell_with_warning#6082
Merged
not522 merged 3 commits intooptuna:masterfrom May 23, 2025
Merged
Conversation
Member
gen740
reviewed
May 21, 2025
optuna/study/_tell.py
Outdated
| if warning_message is not None: | ||
| frozen_trial._system_attrs[STUDY_TELL_WARNING_KEY] = warning_message | ||
| return frozen_trial | ||
| return frozen_trial, warning_message |
Member
There was a problem hiding this comment.
[NIT]
As in this function, warning_message is just an alias for values_conversion_failure_message. The name warning_message does not accurately reflect its actual purpose, which could lead to confusion.
For improved readability and clarity, I suggest changing line 129 to:
values_conversion_failure_message = None
Collaborator
Author
|
@gen740 Thank you for the review. I've made the update accordingly. PTAL! |
not522
reviewed
May 23, 2025
Member
not522
left a comment
There was a problem hiding this comment.
Could you remove tests for warning messages? They are implementation detail and generally are not tested.
Collaborator
Author
|
@not522 Thank you for the advice. I've made the update. PTAL! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The
warning_messagedoes not need to be stored infrozen_trial.system_attrs. This PR refactors the implementation to return the warning message directly from_tell_with_warning.Note: After this change, the warning message will no longer be accessible via
frozen_trial.system_attrs[STUDY_TELL_WARNING_KEY]. However, warning messages can still be captured vialogging.getLogger. In addition, I checked public GitHub repositories, and it looks likeSTUDY_TELL_WARNING_KEYis not used directly by users, so the impact should be minimal.Description of the changes
_tell_with_warninginstead of storing it insystem_attrs.