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 bugs in kurobako/solver/optuna.py #19

Merged
merged 5 commits into from
Apr 13, 2022
Merged

Conversation

contramundum53
Copy link
Member

Several bugs were present in kurobako/solver/optuna.py that resulted in errors when used with nontrivial pruners.

  • trial.last_step is undefined when trial is not a FrozenTrial.
  • self._study._log_completed_trial requries a FrozenTrial, but a Trial is passed
  • Logs for pruning are invisible

These bugs are fixed in this PR.

@@ -11,7 +11,7 @@
from kurobako import problem
from kurobako import solver

_optuna_logger = optuna.logging.get_logger(__name__)
_optuna_logger = optuna.logging.get_logger("optuna.study.study")
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really needed?

kurobako/solver/optuna.py Outdated Show resolved Hide resolved
@HideakiImamura HideakiImamura self-assigned this Apr 13, 2022
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I am wondering if Study._log_completed_trial could be used. Could you explain the change a little more?

Comment on lines 18 to 20
_optuna_logger.setLevel(optuna.logging.INFO)
console = logging.StreamHandler()
_optuna_logger.addHandler(console)
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the necessity of this change?

Copy link
Member

@toshihikoyanase toshihikoyanase Apr 13, 2022

Choose a reason for hiding this comment

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

In my understanding, handlers and log levels are usually set in the user code, not library code.
I don't think it is a good practice to show log messages.

kurobako/solver/optuna.py Outdated Show resolved Hide resolved
@HideakiImamura
Copy link
Member

I privately discuss above with @contramundum53 and @sile. As a result, we have decided to generate a log in our own format for the completed trials in this PR. The reason are

  • to be in the same format as pruned trials, and
  • note to use _log_completed_trial, which is a private method of optuna.

@contramundum53
Copy link
Member Author

We reverted changes about _optuna_logger and made them a separate issue (#20).

Copy link
Member

@sile sile left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@sile sile merged commit 59f0acd into optuna:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants