Instead of testing _extract_raw_model, can we test mlflow.evaluate?

Just because _extract_raw_model works fine doesn't mean it works fine in mlflow.evaluate.

In general, we should test a public API, instead of implementatio details like _extract_raw_model.

Do we need this condition?

This means this function returns None, which can lead to a hard-to-debug error. Just throwing sklearn_model has no attribute predict_proba might easier to debug.

Yes, for this test: https://github.com/mlflow/mlflow/actions/runs/5527963781/jobs/10084241551#step:8:3032
on commit e06bf73

FAILED tests/evaluate/test_default_evaluator.py::test_svm_classifier_evaluation_disable_logging_metrics_and_artifacts - AttributeError: 'LinearSVC' object has no attribute 'predict_proba'

Interesting, how did this test pass before you made the inference params change?

Because self.predict_fn, self.predict_proba_fn = _extract_predict_fn(model, self.raw_model). If the raw_model is the underlying sklearn_model, then from here it is the sklearn model's predict_proba function, instead of the wrapper's predict_proba. But if raw_model is a _SklearnModelWrapper, then self.predict_prob_fn always exist, we might need to update the logic here again since predict_proba_fn = getattr(raw_model, "predict_proba", None) is not None even the function itself returns None 🤔

Or it's fine, because

if self.predict_proba_fn is not None:
    self.y_probs = self.predict_proba_fn(self.X.copy_to_avoid_mutation())
else:
    self.y_probs = None

in the first situation it returns None as well

I don't think we need this test.

What's the case it is not _SklearnModelWrapper instance ? sklearn model._model_impl should be returned by sklearn flavor _load_pyfunc

There's a test case in universe/automl that directly construct a PyFuncModel by passing _model_impl which uses sklearn Pipeline model.

emm... is it a legal usage ?

I guess other flavor might have similar issue if we directly construct _model_impl, but _model_impl is internal attribute managed by pyfunc if I understand it correctly

I think if someone want to customize _model_impl attribute in sklearn pyfunc model,

then it should not be regarded as sklearn pyfunc model but he can create a custom pyfunc model that inherits PythonModel base class.

Can we add a comment on why we need this?

I'm curious about what happens if we evaluate a non-sklearn model like xgboost.

I think there're two problems:

1. mlflow/mlflow/models/evaluation/default_evaluator.py

   Lines 93 to 103 in 4f76275

   	predict_proba_fn = getattr(raw_model, "predict_proba", None)
   	try:
   	import xgboost
   	if isinstance(raw_model, xgboost.XGBModel):
   	# Because shap evaluation will pass evaluation data in ndarray format
   	# (without feature names), if set validate_features=True it will raise error.
   	predict_fn = partial(predict_fn, validate_features=False)
   	if predict_proba_fn is not None:
   	predict_proba_fn = partial(predict_proba_fn, validate_features=False)

   The raw_model will never be xgboost.XGBModel as it will be the wrapper, and predict_prob_fn will be None for all wrappers as they're not implemented.

2. mlflow/mlflow/models/evaluation/default_evaluator.py

   Lines 730 to 732 in 4f76275

   	explainer = shap.Explainer(
   	self.raw_model, sampled_X, feature_names=self.feature_names
   	)

   Looks like shap.Explainer doesn't accept wrappers