Move validation logic from _run_trial to study.tell#3144
Move validation logic from _run_trial to study.tell#3144hvy merged 102 commits intooptuna:masterfrom
_run_trial to study.tell#3144Conversation
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. |
|
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: |
hvy
left a comment
There was a problem hiding this comment.
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
| and not isinstance(func_err, catch) | ||
| ): | ||
| raise func_err | ||
| return trial |
There was a problem hiding this comment.
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.
Please note that the suggested change here is just a part of my suggestion in the comment.
There was a problem hiding this comment.
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.
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.
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.
You suggestion is applied in adecbc4. I'll update message as well.
There was a problem hiding this comment.
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.
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! |
|
I talked with @HideakiImamura and decided to stop moving existing tests to |
HideakiImamura
left a comment
There was a problem hiding this comment.
Thanks for the update. I have several comments around tests. PTAL.
tests/study_tests/test_study.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("storage_mode", STORAGE_MODES) | ||
| def test_run_trial(storage_mode: str) -> None: |
There was a problem hiding this comment.
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.
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: |
tests/study_tests/test_study.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("storage_mode", STORAGE_MODES) | ||
| def test_run_trial_pruned(storage_mode: str) -> None: |
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.
Line is repeated, a small careless.
| logging.enable_propagation() |
|
I ran the local test script and it looked good too. 👍 |
HideakiImamura
left a comment
There was a problem hiding this comment.
I checked all of the test cases. Thanks for the long running work. LGTM!
hvy
left a comment
There was a problem hiding this comment.
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.
Slightly misleading message for a user of Study.tell, but I think it's acceptable.
🔗 #3132
This PR makes the behavior of
optuna tellconsistent withstudy.optimizewhen an objective value isnanor a list containingnan. Insidestudy.optimize, the case observingnanas an objective values is considered as a special case that Optuna doesn't raise an exception and maketrial.statefailed 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 tellconsistent withstudy.optimizeoptuna tellshould only be consistent withstudy.tell. Any feedback is highly appreciatedoptuna tell ... --state failwhen they observenannanis passed tooptuna tell, Optuna makes a trial state FAILEDDescription of the changes
_check_and_convert_to_valuesin_Tell.take_actionBehavior