PUBDEV-7204: GridSearch over TE parameters is not working

[deil87]

This PR fixes two issues:

1. Implementation of TargetEncoderBuilder does not take into account the way how GridSearch is working. Instead of using TargetEncoderBuilder's constructor which sets passed model parameters, GridSearch is using startupOnce constructor with default parameters so that it can later reassign them with values from the grid. Even though initialisation is being called, training frame is not available in default model parameters

2. Wrong key was passed to TargetEncoderModel
   new TargetEncoderModel(_result, ... . Instead of _result from ModelBuilder the one from Job was used. See for more details my comment PUBDEV-7204: GridSearch over TE parameters is not working #4216 (comment)

[deil87]

@michalkurka can we not fail if Lockable is already locked with the same key? This way we can probably include write_lock inside update. I don't know if there are situations when we want to update without prior locking. Maybe local updates do not require distributed locks( then it is better not to include write_lock inside update to give flexibility) .
Of course without failing developers are free to abuse this ability and use write_lock unnecessary number of times. But still it is easier than trying to understand why some execution paths have different lock story. I run into such situation : for GBM model parameters process of building a model does not release lock but for TargetEncoder model parameters it does. But it is hard to understand who and where releases the lock in TE path.

[deil87]

this is outdated. But still interesting to know.

[deil87]

@michalkurka What is the purpose of these two lines? Does it make sure that _targetEncoderModel is updated globally?

[deil87]

job._result was a wrong key to pass to a Model. It was causing key to be removed by Scope. Needed key that is stored in ModelBuilder's _result field first, matches keys that are used to retrieve models from the grid ( this way we don't get N null models in the end) and second, this key is being created outside of (before) Scope.enter() and is not being unnecessary deleted from DKV.

[michalkurka]

I will review this first thing tomorrow - in progress now :)

[michalkurka]

@deil87 tests are great, thank you

Code changes in TargetEncoderModel do not respect how initialization should be done and do not lock model while it is building, my attempt to address these issues: #4245

[deil87]

@michalkurka I also noticed that parallelGridSearch() method does not wrap computations in a try-catch block as gridSearch() does. Maybe we can move this logic up an something like this:

try {
          if (_parallelism == 1) {
            gridSearch(grid);
          } else if (_parallelism > 1) {
            parallelGridSearch(grid);
          } else {
            throw new IllegalArgumentException(String.format("Grid search parallelism level must be >= 1. Give value is '%d'.",
                    _parallelism));
          }
        } finally {
          grid.unlock(_job);
        }

?
and maybe even do `attemptGridSave(grid) before unlocking as well