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
Move validation logic from _run_trial
to study.tell
#3144
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3144 +/- ##
==========================================
+ Coverage 91.83% 91.86% +0.03%
==========================================
Files 156 157 +1
Lines 12292 12316 +24
==========================================
+ Hits 11288 11314 +26
+ Misses 1004 1002 -2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@g-votte and @hvy suggested to move the validation logic inside Lines 577 to 659 in 45caa72
|
Thanks a lot @himkt for quickly addressing the issue with an RFC. Just to clarify, do you mean that making My understanding is that the validation logic (currently only done in |
829b30b
to
c143c3a
Compare
Sorry for the inactive @hvy.
I said it by the point of code complexity. I was not sure how to tell a conversion failure to And I also found Lines 639 to 647 in 10d1111
|
02955ec
to
582dc5b
Compare
nan
in optuna tell
_run_trial
to study.tell
📝 Just had a discussion with @himkt outside this PR. This work is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that this PR makes the behavior of code: This is the same as import optuna
import numpy as np
# In-memory storage
study = optuna.create_study()
trial = study.ask()
try:
study.tell(trial, float(np.nan))
except Exception as e:
print(e)
finally:
print(f"In-memory : Trial state is {study.trials[-1].state}")
# RDB storage (SQLite)
study = optuna.create_study(storage='sqlite:///../sqlite.db')
trial = study.ask()
try:
study.tell(trial, float(np.nan))
except Exception as e:
print(e)
finally:
print(f"SQLite : Trial state is {study.trials[-1].state}")
# RDB storage (MySQL)
study = optuna.create_study(storage="mysql+pymysql://root:test@localhost/mysql")
trial = study.ask()
try:
study.tell(trial, float(np.nan))
except Exception as e:
print(e)
finally:
print(f"MySQL : Trial state is {study.trials[-1].state}")
# RDB storage (PostgreSQL)
study = optuna.create_study(storage="postgresql+psycopg2://postgres:test@localhost:15432/postgres")
trial = study.ask()
try:
study.tell(trial, float(np.nan))
except Exception as e:
print(e)
finally:
print(f"PostgreSQL: Trial state is {study.trials[-1].state}") output:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the code becomes simpler with this change in my opinion. Left some comments still if you could take a look. 🙏
optuna/study/_optimize.py
Outdated
frozen_trial.state == TrialState.FAIL | ||
and func_err is not None | ||
and not isinstance(func_err, catch) | ||
): | ||
raise func_err | ||
return trial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify the internal logic even further (at _optimize_sequential
, the caller of _run_trial
) now with the return value of Study.tell
.
return trial | |
return frozen_trial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that the suggested change here is just a part of my suggestion in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, you can simplify it even further. There is a line in _optimize_sequential
that obtains a frozen trial from the previously returned Trial. Now that we return a frozen trial, we can skip this line.
frozen_trial = copy.deepcopy(study._storage.get_trial(trial._trial_id))
optuna/study/study.py
Outdated
if state is None and values is None: | ||
self._storage.set_trial_state(trial_id, TrialState.FAIL) | ||
_logger.warning("You must specify either state or values.") | ||
return self._storage.get_trial(trial_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this always be executed without taking into account skip_if_finished
? Wondering if something like the following can replace.
if state is None and values is None: | |
self._storage.set_trial_state(trial_id, TrialState.FAIL) | |
_logger.warning("You must specify either state or values.") | |
return self._storage.get_trial(trial_id) | |
if state is None and values is None: | |
state = TrialState.FAIL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging message might be lacking information for the user, I think adding some context would help. (Besides from the comment above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You suggestion is applied in adecbc4. I'll update message as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the doc in 6fa50b4. I'm not sure if it improves a usability so please give me a feedback. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Sorry for my vague comment but I meant that we could omit the warning altogether, if we get rid of the early return statement (which you did).
cb6e986
to
03d80ea
Compare
Thank you so much @hvy for the careful review. I revised my PR based on your comments/suggestions! |
Thanks a lot, I'll go through the changes today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment
I talked with @HideakiImamura and decided to stop moving existing tests to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I have several comments around tests. PTAL.
tests/study_tests/test_study.py
Outdated
@@ -912,6 +913,66 @@ def objective(trial: Trial) -> float: | |||
assert states == [] | |||
|
|||
|
|||
@pytest.mark.parametrize("storage_mode", STORAGE_MODES) | |||
def test_run_trial(storage_mode: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this test should be placed in test_optimize.py
since this does not call Study.optimize
but _run_trial
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally right, sorry for my careless. 🙇 8d62268
tests/study_tests/test_study.py
Outdated
|
||
|
||
@pytest.mark.parametrize("storage_mode", STORAGE_MODES) | ||
def test_run_trial_automatically_fail(storage_mode: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
tests/study_tests/test_study.py
Outdated
|
||
|
||
@pytest.mark.parametrize("storage_mode", STORAGE_MODES) | ||
def test_run_trial_pruned(storage_mode: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
After my approval, the codes ware changed. I will re-review.
https://github.com/optuna/optuna/runs/5813743672?check_suite_focus=true#step:8:128 |
3bd7bd3 solves. @hvy @HideakiImamura PTAL. 🙏 |
tests/study_tests/test_optimize.py
Outdated
|
||
# Test trial with invalid objective value: None | ||
def func_none(_: Trial) -> float: | ||
logging.enable_propagation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is repeated, a small careless.
logging.enable_propagation() |
I ran the local test script and it looked good too. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all of the test cases. Thanks for the long running work. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you very much for the effort. 🙇
While code complexity is still somewhat high, it turned out a lot better than expected.
Tests look great in my opinion and should make future refactoring a lot easier.
There are some nits such as commenting about the in-memory system attribute for warning propagation, and maybe a request to check in unit tests that when Study.tell
fails with e.g. ValueError
, that a trial is not modified. These are not significant, or not something introduced by this PR.
|
||
if value is not None and math.isnan(value): | ||
value = None | ||
failure_message = f"The objective function returned {original_value}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly misleading message for a user of Study.tell
, but I think it's acceptable.
🔗 #3132
This PR makes the behavior of
optuna tell
consistent withstudy.optimize
when an objective value isnan
or a list containingnan
. Insidestudy.optimize
, the case observingnan
as an objective values is considered as a special case that Optuna doesn't raise an exception and maketrial.state
failed and show a log message. For Optuna CLI, I think it would be natural to behave as instudy.optimize
.optuna/optuna/study/_optimize.py
Lines 224 to 230 in cf33d05
optuna/optuna/study/_optimize.py
Lines 256 to 257 in cf33d05
Motivation
optuna tell
consistent withstudy.optimize
optuna tell
should only be consistent withstudy.tell
. Any feedback is highly appreciatedoptuna tell ... --state fail
when they observenan
nan
is passed tooptuna tell
, Optuna makes a trial state FAILEDDescription of the changes
_check_and_convert_to_values
in_Tell.take_action
Behavior