Use a fixed seed value when seed=None in GridSampler

[nzw0301]

Motivation

Resolve #5489. I'm happy to hear the opinion on this change because I'm not sure why this shuffle was introduced.

Description of the changes

Remove the shuffle.

For distributed optimisation, optuna docs suggest not setting seed for samplers or set different values. In this PR, call shuffle when seed=0.

[eukaryo]

FYI: This shuffling is introduced in PR #4918.

[eukaryo]

When I introduced this PR, I believed that GridSampler should search in a random order. This is because when n_trials < len(self._all_grids), there is no guarantee that the regions searched earlier will have good values. In such cases, RandomSampler tends to evaluate the same point multiple times, whereas GridSampler (with shuffling) is expected to evaluate each point once without duplication. Of course, I agree that the issue should be resolved. However, I believe that the lack of a shuffling option results in a performance loss.

[eukaryo]

I have suggested a change. PTAL.

[eukaryo]

@not522 could you review this PR? I suppose you are the appropriate reviewer because you were also a reviewer of PR #4918.

[nzw0301]

Thank you for your explanation! I agree with you to shuffle the grid!

[nzw0301]

Should I add a note or update seed docstring on this? This is still issue when user specify different random seeds among processes.

[eukaryo]

Yes, it would be a great help if you could do that.

[nzw0301]

Thanks. Sure!

[eukaryo]

LGTM!

[not522]

This PR makes the order of trials fixed for seed=None. It will be a breaking change, so I'm thinking how to resolve #5489 without this problem...

[eukaryo]

In my current understanding, the requirements can be organized and enumerated as follows:

1. If seed=None, search order should be shuffled. (From the historical background; initial implementation bahaves that way.)
2. If seed=None, search order should be shuffled differently each time. (Because someone may have set up an experimental system that assumes this (expecially in a non-distributed environment).)
3. If seed=None, duplicates should not occur in a distributed environment (issue#5489).
4. _get_unvisited_grid_ids calls get_all_trials internally, which is heavy and I wanted to avoid calling it (PR#4918). Additionally, the process including _get_unvisited_grid_ids is not atomic, so it cannot prevent duplication perfectly.

The second request means that the self._rng = LazyRandomState(seed) should be resulted identical across all processes of any distibuted environment when seed=None.

If we want all processes to have an unique value in a distributed environment, the only way is to share the value using storage and appropriate transaction process. The only time when this is possible is not during __init__, but during before_trial, because it needs study received as an argument. Therefore, it is simplest to perform transaction processing and shuffling as necessary at the time of before_trial.

This discussion is somewhat complicated, but since it involves historical circumstances and a distributed environment, I believe such complexity is inevitable and unavoidable.

To implement the above plan, we need to enable set_study_system_attr to do "as single transaction, write the value only if the key does not exist, otherwise do nothing". It is necessary to add an optional argument (or new methods) to BaseStorage and all derived storages.

[eukaryo]

After discussion with @not522 -san and others, I have reconsidered the second of the above requirements is not necessary. (It is a little strange that only GridSampler behaves deterministic with seed=None while other Samplers etc. behave stochastic, but I think it is acceptable if explained in the docstring.) Adding functionality to the storage module should be done with the most caution and should not be tied to this PR, nor is it necessary.
Partial revert of PR #4918 is another option, but then Issue #4627 will recur.

[nabenabe0928]

Thank you for the PR!
I added a suggestion to the doc-string.

[nabenabe0928]

@eukaryo @not522
Do you think we can remove this TODO comment?

• https://github.com/optuna/optuna/pull/5490/files#diff-87630c6e088d5b9840e0ec55c5c4b4b813f653d780f41d8ef889646578d5ba0aL197-L199

[nzw0301]

Thank you for your comments and discussions. I've updated the PR by following the suggestions.

[nabenabe0928]

Thank you for the changes!
I added another nit comment.

[not522]

Thank you for your update. It looks good to me.
@nabenabe0928 If you approve this PR, please merge it.

[nabenabe0928]

LGTM!