Latest release breaks MCMCDiagnosticTools

[devmotion]

It seems MLJBase 0.21, and in particular the changes to predict for Pipelines, break MCMCDiagnosticTools since the output of predict is not always the desired predictions but sometimes also a tuple of predictions and something else now (see, e.g., TuringLang/MCMCDiagnosticTools.jl#44).

using MLJBase
using MLJXGBoostInterface
using DataFrames
using Test

iris = DataFrame(load_iris())
y, X = unpack(iris, ==(:target))
train, test = partition(eachindex(y), 0.7)

XGBoostClassifier = @load XGBoostClassifier pkg=XGBoost

model1 = XGBoostClassifier()
fitresult, _ = fit(model1, 0, X[train, :], y[train])
@test predict(model1, fitresult, X[test, :]) isa UnivariateFiniteVector

model2 = Pipeline(XGBoostClassifier(); operation=predict_mode)
fitresult, _ = fit(model2, 0, X[train, :], y[train])
@test predict(model2, fitresult, X[test, :]) isa AbstractVector{<:MLJBase.CategoricalValue}

This example works with MLJBase < 0.21 but in MLJBase 0.21 the last line returns a tuple where the first element are the desired predictions.

Some more details about our use case:
In MCMCDiagnosticTools we do want to keep dependencies minimal and hence depend only on MLJModelInterface. Therefore we use fit and predict instead of fit! and predict! with machines of models.
Additionally, models are provided by users and depending on whether the output of predict is a vector of distributions or not, we dispatch internally to one of two algorithms (https://github.com/TuringLang/MCMCDiagnosticTools.jl/blob/5e3232f500d10bf352ced88530e3f07e3d132ab3/src/rstar.jl#L21-L24).
Users are supposed to provide classifiers with deterministic predictions if they want to use one algorithm, and with probabilistic predictions if they want to use the other algorithm.
Hence the ability for users to change a probabilistic classifier to a deterministic one is crucial if they want to use e.g. XGBoost with the algorithm for deterministic predictions.
In previous versions of MLJ and MLJBase this was possible by defining a Pipeline with e.g. predict_mode as operation but as the example shows this is not possible anymore with MLJBase 0.21 it seems.

[OkonSamuel]

@devmotion Yes, there was a breaking change in the API to make operations (e.g predict, transform) capable of modifying a machines report. This is useful for certain Static unsurpervised models.
@ablaom correct me if I'm wrong. I believe now there are two types of outputs from calling operations (e.g predict, transform, etc.) on a MLJModelInterface.Model.

• a single output , for the case where in(operation, MLJModelInterface.reporting_operations(x)) returns false
• Two-Tuple output of the form (result, report) for the case where where in(operation, MLJModelInterface.reporting_operations(x)) returns true e.g NetworkComposites

To fix your issue you just need to define the following method.

_actual_predictions(x) = x
_actual_predictions(x::Tuple) = first(x)

or better still

function _actual_predictions(x::MLJModelInterface.Model)
    if :predict in MLJModelInterface.reporting_operations(x) #This check is done at compile time
        return first(x)
    else
        return x
    end
end

Then replace https://github.com/TuringLang/MCMCDiagnosticTools.jl/blob/5e3232f500d10bf352ced88530e3f07e3d132ab3/src/rstar.jl#L96 with

predictions = _actual_predictions(MLJModelInterface.predict(classifier, fitresult, xtest))

[devmotion]

To fix your issue you just need to define the following method.

Sure, that was the first thing I wanted to do. But it felt wrong that every downstream package that wants to work with predictions has to do that now and it's a regression. I think this is a bug and there should be an API in MLJModelinterface that makes it possible to only obtain the desired predictions, as it was possible with predict before. Maybe a new function is needed? Or maybe predict should be reverted to the old behaviour and a new function should be used for the tuple output? I think the latter would seem more natural but I don't know anything about MLJ's internals or design decisions 🤷‍♂️

[ablaom]

@devmotion Thanks for looking into this.

@OkonSamuel has the gist of it. The missing information is the existence of the trait reporting_operations that allows you to detect the new phenomenon.

Since MLJModelInterface 1.6, an implementation of MLJ's model API has had the option of overloading a new model trait MLJModelInterface.reporting_operations, which is a tuple of operation names (symbols) such as :predict, :transform, etc (details). If overloaded, then that is a promise that the listed operations return two objects instead of one, the second object being a "report" item. Of course, the fallback is an empty list, otherwise this would have been a breaking change.

using MLJModels, MLJBase, MLJModelInterface
DBSCAN = @iload DBSCAN pkg=Clustering
julia> MLJModelInterface.reporting_operations(DBSCAN())
(:predict,)

For Pipeline this trait used to return () but in MLJBase 0.21 it returns all operations:

pipe = OneHotEncoder() |> Standardizer() 

julia> MLJModelInterface.reporting_operations(pipe)
(:predict, :predict_mean, :predict_mode, :predict_median, :predict_joint, :transform, :inverse_transform)

So, post MLJModelInterface 1.6, one should test the value of the reporting_operations trait to see how to appropriately process the output of an operation.

@devmotion Does that help?

[DilumAluthge]

It seems like this should have been a breaking change in MLJModelInterface?

Also, it seems confusing to me that predict sometimes returns the predictions and other times returns a tuple of predictions and a report. Personally, I think it would be more intuitive if predict always returned only the predictions, and then there was a second function predict_report that would always return:

1. (predictions, report) if the model supports returning a report
2. (precitions, nothing) if the model doesn't support returning a report

This also allows defining predict(...) as just predict_report(...)[1] because the length of predict_report is always two.

[devmotion]

Thanks, that explains the underlying changes and problems!

So, post MLJModelInterface 1.6, one should test the value of the reporting_operations trait to see how to appropriately process the output of an operation.

In our use-case, and I would assume more generally also for other uses, this seems quite inconvenient. It seems a lot easier to work with a more stable API that does not have to be coupled with manual checks that - it seems - also rely on constant propagation. I am sure there is a reason for this design choice but I would have preferred if predict etc. always consistently returns the same thing for any model - namely the predictions. And if there would be a separate syntax for obtaining both predictions and reports, such as predict_reports or with_reports(predict, ...), that would always return a tuple of predictions and reports (and fall back to just returning (predictions, nothing) or some other tuple with a "no report").

[ablaom]

@devmotion @DilumAluthge I think these are fair criticisms, and the feedback is appreciated. In order to address a long-standing problem with non-generalizing models, these changes were formulated, as they seemed the least disruptive, and I still believe this to be the case. Unless further issues come to light, I don't see further action on the MLJ side is going to be helpful.

Mostly, MLJModelInterface exists as a collection of methods for new MLJ models to implement, but the methods themselves (predict(model, ...), fit(model, ...), etc) weren't intended to be public. Rather the "machine" API is the public one. In retrospect, that was a bad idea.

I am currently working on a proposal for machine learning API that would more properly stand-alone from MLJModelInterface.jl, and which would replace StatisticalTraits.jl and parts of MLJModelInterface in a significantly breaking way. At present, in that proposal, all operations return both a "output" and a "report" (typically nothing) as @DilumAluthge has suggested above. Perhaps further design discussions can be moved to an issue there, thanks.

[ablaom]

@DilumAluthge Just to clarify, in case there was confusion, calling predict on a machine, still gives predictions only. The changes only affected predict when called on a model, which the ordinary user does not do.

[devmotion]

Just to clarify, in case there was confusion, calling predict on a machine, still gives predictions only.

Yes, before opening the issue I had checked and noticed that machines still work as expected. Maybe I should have mentioned that more clearly, sorry for the confusion.

Mostly, MLJModelInterface exists as a collection of methods for new MLJ models to implement, but the methods themselves (predict(model, ...), fit(model, ...), etc) weren't intended to be public. Rather the "machine" API is the public one.

Unfortunately, the machine API is not really an option for downstream packages such as MCMCDiagnosticTools that want to keep dependencies minimal. The general trend in the community seems to move towards creating and using more and more interface packages to reduce the dependency structure and support additional non-core features. I assumed that MLJModelInterface (a "Lightweight package to interface with MLJ") is the preferred solution for downstream packages that want to interface with MLJ without pulling in all the dependencies of e.g. MLJBase.

---

I added a workaround to MCMCDiagnosticTools (https://github.com/TuringLang/MCMCDiagnosticTools.jl/pull/44/files) but since I still think this should be fixed in MLJ rather than downstream packages, it would be great if this issue would be tracked in MLJ, either here or in some other repo.

[ablaom]

I think we can consider the LearnAPI.jl proposal as "tracking" this issue. This issue will not be resolved by any incremental changes to MLJModelInterface.