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

Warn multiple intermediate value reports with the same step. #852

Closed
sile opened this issue Jan 22, 2020 · 9 comments · Fixed by #2782
Closed

Warn multiple intermediate value reports with the same step. #852

sile opened this issue Jan 22, 2020 · 9 comments · Fixed by #2782
Labels
contribution-welcome Issue that welcomes contribution. feature Change that does not break compatibility, but affects the public interfaces. good first issue Good first issue for contribution. no-stale Exempt from stale bot

Comments

@sile
Copy link
Member

sile commented Jan 22, 2020

Currently, if trial.report() method is called multiple times with the same step in a trial, only the first reported value will be recorded into the storage and the others will be silently ignored.
Reporting multiple values with the same step is an unintended usage of the method, and it could cause bugs that are hard to notice (see #847 as an example of such bug).
Therefore, it seems better to add a check to detect the duplicate reports, and if it's detected, we should emit a warning message.

As an implementation note, this modification could be done without any additional DB costs (see #847 (comment) for the detail).

@sile sile added feature Change that does not break compatibility, but affects the public interfaces. contribution-welcome labels Jan 22, 2020
@sile sile added contribution-welcome Issue that welcomes contribution. and removed contribution-welcome labels Feb 7, 2020
@hvy hvy added the good first issue Good first issue for contribution. label Feb 14, 2020
@stakky
Copy link

stakky commented Feb 14, 2020

@sile Hi. Can I take this issue?

@sile
Copy link
Member Author

sile commented Feb 14, 2020

Of course, please go for it!

@stakky
Copy link

stakky commented Feb 20, 2020

@sile
I implemanted a prototype and tested it. I changed a pruning sample(optuna/examples/pruning/simple.py) to reproduce the issue and then warning message was emitted as intended.
stakky@d42fc64
But this prototype emits the warning message too much. I think it should be emited per one trial. What do you think?

@sile
Copy link
Member Author

sile commented Feb 21, 2020

@stakky Thanks!

But this prototype emits the warning message too much. I think it should be emited per one trial.

I agree with your opinion. I think that it's better to use warnings.warn() instead of Optuna logger. warnings.warn() don't emit duplicate messages.
The detail has not been decided, but Optuna dev team is planning to use warnings.warn() for warnings related to implementation miss, and Optuna logger for warnings related to experimentation (e.g., trial failure report).

@sile
Copy link
Member Author

sile commented Feb 21, 2020

Anyway, if you create a (WIP or draft) PR, we can discuss it on the PR.

@github-actions
Copy link
Contributor

This issue has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Apr 18, 2020
@hvy hvy removed the stale Exempt from stale bot labeling. label May 8, 2020
@hvy hvy added the no-stale Exempt from stale bot label Jun 12, 2020
@hvy
Copy link
Member

hvy commented Aug 4, 2020

Still seems to be an open issue.

@norihitoishida
Copy link
Contributor

Let me tackle this issue!

@TakuyaInoue-github
Copy link
Contributor

I'm sorry for interrupting but there is no activity on the issue. so i tuckle this issue and create PR.
Please check and comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution-welcome Issue that welcomes contribution. feature Change that does not break compatibility, but affects the public interfaces. good first issue Good first issue for contribution. no-stale Exempt from stale bot
Projects
None yet
6 participants