Repository: JuliaAI/MLJBase.jl (julia)

Issue:
Serialized Composite Model Fails with XGBoost
I am having an issue with a serialized MLJ pipeline that includes an XGBoostClassifier - when trying to run predict  on the loaded model, I get the following error:
```
┌ Error: Failed to apply the operation `predict` to the machine machine(:xg_boost_classifier, …), which receives it's data arguments from one or more nodes in a learning network. Possibly, one of these nodes is delivering data that is incompatible with the machine's model.
│ Model (xg_boost_classifier):
│ input_scitype = Unknown
│ target_scitype =Unknown
│ output_scitype =Unknown
│
│ Incoming data:
│ arg of predict	scitype
│ -------------------------------------------
│ Node @103	Table{AbstractVector{Continuous}}
│
│ Learning network sources:
│ source	scitype
│ -------------------------------------------
│ Source @253	Nothing
│ Source @396	Nothing
└ @ MLJBase ~/.julia/packages/MLJBase/0rn2V/src/composition/learning_networks/nodes.jl:153
ERROR: XGBoostError: (caller: XGBoosterPredictFromDMatrix)
[00:01:27] /workspace/srcdir/xgboost/src/c_api/c_api.cc:913: Booster has not been initialized or has already been disposed.
Stack trace:
  [bt] (0) 1   libxgboost.dylib                    0x000000028780cc4c dmlc::LogMessageFatal::~LogMessageFatal() + 124
  [bt] (1) 2   libxgboost.dylib                    0x0000000287827614 XGBoosterPredictFromDMatrix + 116
  [bt] (2) 3   ???                                 0x00000002a858c0c8 0x0 + 11414323400
  [bt] (3) 4   ???                                 0x00000002a8404194 0x0 + 11412717972
  [bt] (4) 5   libjulia-internal.1.9.dylib         0x00000001048cdacc do_apply + 744
  [bt] (5) 6   ???                                 0x00000002a802c1a0 0x0 + 11408687520
  [bt] (6) 7   ???                                 0x00000002a79c40d8 0x0 + 11401969880
  [bt] (7) 8   ???                                 0x00000002a78e40d4 0x0 + 11401052372
  [bt] (8) 9   libjulia-internal.1.9.dylib         0x00000001048da53c do_call + 188
```

I've noted this on both Julia 1.8.5 and Julia 1.9.2 using up-to-date libraries for MLJ and XGBoost (which I believe recently updated to a version 2.0.0).  I also have noted that recently, the `MLJXGBoostInterface` library changed the way they serialize the XGBoost models.

Pull request description:
This PR resolves #927.

Patch:
diff --git a/Project.toml b/Project.toml
index 34046493..ec42fc93 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,7 +1,7 @@
 name = "MLJBase"
 uuid = "a7f614a8-145f-11e9-1d2a-a57a1082229d"
 authors = ["Anthony D. Blaom <anthony.blaom@gmail.com>"]
-version = "1.1.1"
+version = "1.1.2"
 
 [deps]
 CategoricalArrays = "324d7699-5711-5eae-9e2f-1d82baa6b597"
diff --git a/src/composition/learning_networks/replace.jl b/src/composition/learning_networks/replace.jl
index 175c4b91..08145262 100644
--- a/src/composition/learning_networks/replace.jl
+++ b/src/composition/learning_networks/replace.jl
@@ -13,9 +13,9 @@ the `model` and `args` field values as derived from the provided dictionaries. I
 the returned machine is hooked into the new learning network defined by the values of
 `newnode_given_old`.
 
-If `serializable=true`, return a serializable copy instead (namely,
-`serializable(node.mach)`) and ignore the `newmodel_given_old` dictionary (no model
-replacement).
+If `serializable=true`, return a serializable copy instead, but make no model replacement.
+The `newmodel_given_old` dictionary is still used, but now to look up the concrete model
+corresponding to the symbolic one stored in `node`'s machine.
 
 See also [`serializable`](@ref).
 
@@ -26,9 +26,10 @@ function machine_replacement(
     newnode_given_old,
     serializable
 )
-    # the `replace` called here is defined in src/machines.jl:
-    mach = serializable ? MLJBase.serializable(N.machine) :
-        replace(N.machine, :model => newmodel_given_old[N.machine.model])
+    # the `replace` called below is defined in src/machines.jl.
+    newmodel = newmodel_given_old[N.machine.model]
+    mach = serializable ? MLJBase.serializable(N.machine, newmodel) :
+        replace(N.machine, :model => newmodel)
     mach.args = Tuple(newnode_given_old[arg] for arg in N.machine.args)
     return mach
 end
@@ -38,6 +39,7 @@ end
         newnode_given_old,
         newmach_given_old,
         newmodel_given_old,
+        serializable,
         node::AbstractNode)
 
 **Private method.**
@@ -86,9 +88,11 @@ const DOC_REPLACE_OPTIONS =
     - `copy_unspecified_deeply=true`: If `false`, models or sources not listed for
       replacement are identically equal in the original and returned node.
 
-    - `serializable=false`: If `true`, all machines in the new network are serializable.
-      However, all `model` replacements are ignored, and unspecified sources are always
-      replaced with empty ones.
+    - `serializable=false`: If `true`, all machines in the new network are made
+      serializable and the specified model replacements are only used for serialization
+      purposes: for each pair `s => model` (`s` assumed to be a symbolic model) each
+      machine with model `s` is replaced with `serializable(mach, model)`. All unspecified
+      sources are always replaced with empty ones.
 
     """
 
@@ -192,7 +196,7 @@ function _replace(
 
     # Instantiate model dictionary:
     model_pairs = filter(collect(pairs)) do pair
-        first(pair) isa Model
+        first(pair) isa Model || first(pair) isa Symbol
     end
     models_ = models(W)
     models_to_copy = setdiff(models_, first.(model_pairs))
diff --git a/src/composition/models/network_composite.jl b/src/composition/models/network_composite.jl
index f5586950..dcec4f83 100644
--- a/src/composition/models/network_composite.jl
+++ b/src/composition/models/network_composite.jl
@@ -88,18 +88,33 @@ MLJModelInterface.fitted_params(composite::NetworkComposite, signature) =
 MLJModelInterface.reporting_operations(::Type{<:NetworkComposite}) = OPERATIONS
 
 # here `fitresult` has type `Signature`.
-save(model::NetworkComposite, fitresult) = replace(fitresult, serializable=true)
+function save(model::NetworkComposite, fitresult)
+    # The network includes machines with symbolic models. These machines need to be
+    # replaced by serializable versions, but we cannot naively use `serializable(mach)`,
+    # because the absence of the concrete model means this just returns `mach` (because
+    # `save(::Symbol, fitresult)` returns `fitresult`). We need to use the special
+    # `serialiable(mach, model)` instead. This is what `replace` below does, because we
+    # pass it the flag `serializable=true` but we must also pass `symbol =>
+    # concrete_model` replacements, which we calculate first:
+
+    greatest_lower_bound = MLJBase.glb(fitresult)
+    machines_given_model = MLJBase.machines_given_model(greatest_lower_bound)
+    atomic_models = keys(machines_given_model)
+    pairs = [atom => getproperty(model, atom) for atom in atomic_models]
+
+    replace(fitresult, pairs...; serializable=true)
+end
 
 function MLJModelInterface.restore(model::NetworkComposite, serializable_fitresult)
     greatest_lower_bound = MLJBase.glb(serializable_fitresult)
     machines_given_model = MLJBase.machines_given_model(greatest_lower_bound)
-    models = keys(machines_given_model)
+    atomic_models = keys(machines_given_model)
 
     # the following indirectly mutates `serialiable_fiteresult`, returning it to
     # usefulness:
-    for model in models
-        for mach in machines_given_model[model]
-            mach.fitresult = restore(model, mach.fitresult)
+    for atom in atomic_models
+        for mach in machines_given_model[atom]
+            mach.fitresult = MLJBase.restore(getproperty(model, atom), mach.fitresult)
             mach.state = 1
         end
     end
diff --git a/src/machines.jl b/src/machines.jl
index a1a3afc5..5b2e0fd0 100644
--- a/src/machines.jl
+++ b/src/machines.jl
@@ -975,17 +975,17 @@ A machine returned by `serializable` is characterized by the property
 See also [`restore!`](@ref), [`MLJBase.save`](@ref).
 
 """
-function serializable(mach::Machine{<:Any, C}; verbosity=1) where C
+function serializable(mach::Machine{<:Any, C}, model=mach.model; verbosity=1) where C
+
+    isdefined(mach, :fitresult) || throw(ERR_SERIALIZING_UNTRAINED)
+    mach.state == -1 && return mach
 
     # The next line of code makes `serializable` recursive, in the case that `mach.model`
     # is a `Composite` model: `save` duplicates the underlying learning network, which
     # involves calls to `serializable` on the old machines in the network to create the
     # new ones.
 
-    isdefined(mach, :fitresult) || throw(ERR_SERIALIZING_UNTRAINED)
-    mach.state == -1 && return mach
-
-    serializable_fitresult = save(mach.model, mach.fitresult)
+    serializable_fitresult = save(model, mach.fitresult)
 
     # Duplication currenty needs to happen in two steps for this to work in case of
     # `Composite` models.
@@ -1017,9 +1017,9 @@ useable form.
 For an example see [`serializable`](@ref).
 
 """
-function restore!(mach::Machine)
+function restore!(mach::Machine, model=mach.model)
     mach.state != -1 && return mach
-    mach.fitresult = restore(mach.model, mach.fitresult)
+    mach.fitresult = restore(model, mach.fitresult)
     mach.state = 1
     return mach
 end


Test patch:
diff --git a/test/machines.jl b/test/machines.jl
index 7d0845c2..c78aa06d 100644
--- a/test/machines.jl
+++ b/test/machines.jl
@@ -508,6 +508,80 @@ end
     rm(filename)
 end
 
+# define a model with non-persistent fitresult:
+thing = []
+struct EphemeralTransformer <: Unsupervised end
+function MLJModelInterface.fit(::EphemeralTransformer, verbosity, X)
+    view = pointer(thing)
+    fitresult = (thing, view)
+    return fitresult, nothing, NamedTuple()
+end
+function MLJModelInterface.transform(::EphemeralTransformer, fitresult, X)
+    thing, view = fitresult
+    return view == pointer(thing) ? X : throw(ErrorException("dead fitresult"))
+end
+function MLJModelInterface.save(::EphemeralTransformer, fitresult)
+    thing, _ = fitresult
+    return thing
+end
+function MLJModelInterface.restore(::EphemeralTransformer, serialized_fitresult)
+    view = pointer(thing)
+    return (thing, view)
+end
+
+# commented out code just tests the transformer above has desired properties for testing:
+
+# # test model transforms:
+# model = EphemeralTransformer()
+# mach = machine(model, 42) |> fit!
+# @test MLJBase.transform(mach, 27) == 27
+
+# # direct serialization fails:
+# io = IOBuffer()
+# serialize(io, mach)
+# seekstart(io)
+# mach2 = deserialize(io)
+# @test_throws ErrorException("dead fitresult") transform(mach2, 42)
+
+@testset "serialization for model with non-persistent fitresult" begin
+    X = (; x=randn(5))
+    mach = machine(EphemeralTransformer(), X)
+    fit!(mach, verbosity=0)
+    v = MLJBase.transform(mach, X).x
+    io = IOBuffer()
+    serialize(io, serializable(mach))
+    seekstart(io)
+    mach2 = restore!(deserialize(io))
+    @test MLJBase.transform(mach2, X).x == v
+
+    # using `save`/`machine`:
+    MLJBase.save(io, mach)
+    seekstart(io)
+    mach2 = machine(io)
+    @test MLJBase.transform(mach2, X).x == v
+end
+
+@testset "serialization for model with non-persistent fitresult in pipeline" begin
+    # https://github.com/JuliaAI/MLJBase.jl/issues/927
+    X = (; x=randn(5))
+    pipe =  Standardizer |> EphemeralTransformer
+    X = (; x=randn(5))
+    mach = machine(pipe, X)
+    fit!(mach, verbosity=0)
+    v = MLJBase.transform(mach, X).x
+    io = IOBuffer()
+    serialize(io, serializable(mach))
+    seekstart(io)
+    mach2 = restore!(deserialize(io))
+    @test MLJBase.transform(mach2, X).x == v
+
+    # using `save`/`machine`:
+    MLJBase.save(io, mach)
+    seekstart(io)
+    mach2 = machine(io)
+    @test MLJBase.transform(mach2, X).x == v
+end
+
 struct ReportingDynamic <: Unsupervised end
 MLJBase.fit(::ReportingDynamic, _, X) = nothing, 16, NamedTuple()
 MLJBase.transform(::ReportingDynamic,_, X) = (X, (news=42,))
diff --git a/test/resampling.jl b/test/resampling.jl
index e2169ec0..d27af319 100644
--- a/test/resampling.jl
+++ b/test/resampling.jl
@@ -117,6 +117,15 @@ API.@trait(
                                    [LogLoss(), ], dummy_interval, 1))
 end
 
+@everywhere begin
+    nfolds = 6
+    nmeasures = 2
+    func(mach, k) = (
+        (sleep(MLJBase.PROG_METER_DT*rand(rng)); fill(1:k, nmeasures)),
+        :fitted_params,
+        :report,
+    )
+end
 @testset_accelerated "dispatch of resources and progress meter" accel begin
 
     @info "Checking progress bars:"
@@ -124,15 +133,6 @@ end
     X = (x = [1, ],)
     y = [2.0, ]
 
-    @everywhere begin
-        nfolds = 6
-        nmeasures = 2
-        func(mach, k) = (
-            (sleep(MLJBase.PROG_METER_DT*rand(rng)); fill(1:k, nmeasures)),
-            :fitted_params,
-            :report,
-        )
-    end
     mach = machine(ConstantRegressor(), X, y)
     if accel isa CPUThreads
         result = MLJBase._evaluate!(
@@ -643,15 +643,15 @@ end
 
 struct DummyResamplingStrategy <: MLJBase.ResamplingStrategy end
 
-@testset_accelerated "custom strategy depending on X, y" accel begin
-    function MLJBase.train_test_pairs(resampling::DummyResamplingStrategy,
-                              rows, X, y)
-        train = filter(rows) do j
-            y[j] == y[1]
+function MLJBase.train_test_pairs(resampling::DummyResamplingStrategy,
+                                  rows, X, y)
+    train = filter(rows) do j
+        y[j] == y[1]
         end
-        test = setdiff(rows, train)
-        return [(train, test),]
-    end
+    test = setdiff(rows, train)
+    return [(train, test),]
+end
+@testset_accelerated "custom strategy depending on X, y" accel begin
 
     X = (x = rand(rng,8), )
     y = categorical(string.([:x, :y, :x, :x, :y, :x, :x, :y]))