Move validation of Sklearn inference_args to construction time

[yeandy]

What would you like to happen?

Sklearn RunInference, at execution time, verifies that no inference_args are passed. Make a change to shorten feedback loop by at doing verification at construction time.

Discussion: #21806 (comment)

Parent Issue: #21435

Issue Priority

Priority: 2

Issue Component

Component: sdk-py-core

[ryanthompson591]

Since this is a side input of run_inference, it's not possible to know at construction time of ModelHandler, but we could know when RunInference is created I think.

Idea 1:

Make a method in ModelHandler:

def framework_expects_parameters():
  # Override this to be true if your framework expects parameters.
  return False

Then in base.py we could validate. This should happen in the runner instead of the worker and be very fast. This should also be less code, since I saw nVidia copy this validation into their implementation.

Idea 2

Just get rid of this validation, passing in parameters to a framework that can't use them could be a no op. Right now this is what I plan to do in TFX-BSL.

Think of it this way -- If we add more optional parameters in the future. It's not possible for all frameworks to be constantly updating to check that named parameters that they don't use and don't expect are not populated. In fact, no op should be the assumption in these cases.

Idea 3

Move this validation code into base.py... since nVidia and future frameworks are just copy-pasting this code why not make it available.

Idea 4

my preference

pass in any optional arguments only if they are present:

In base.py _RunInferenceDoFn becomes this

  def process(self, batch, inference_args):
    start_time = _to_microseconds(self._clock.time_ns())
    extra_kwargs = {}
    if inference_args:
     extra_kwargs[inference_args] = inference_args
    result_generator = self._model_handler.run_inference(
        batch, self._model, **extra_kwargs)
    predictions = list(result_generator)

This has the advantage that it can scale to any amount of optional named arguments, and implementations that don't expect the extra args will just fail with an unexpected arg exception as expected.

@tvalentyn @yeandy any preference?

[yeandy]

1. Seems like an extra friction point as it will be an additional thing users would have to learn and implement. Doable, but prefer not to do this over choice 2 (see below).
2. I think a no-op would be the easiest to for others to use/understand. We can add addition documentation explaining how this parameter is optional to use.
3. If it's moved into base.py, could you illustrate how would we make the logic different for the different frameworks?
4. Would this cause compatibility issues with the existing interface for the run_inference function since it expects inference_args?

  def run_inference(
      self,
      batch: Sequence[ExampleT],
      model: ModelT,
      inference_args: Optional[Dict[str, Any]] = None) -> Iterable[PredictionT]:

[ryanthompson591]

.take-issue