Mlj.jl: `@load` should only do import or using, not define a `const`

[ExpandingMan]

The current behavior of @load is, for example

julia> @macroexpand @load RandomForestRegressor pkg=DecisionTree
[ Info: For silent loading, specify `verbosity=0`.
┌ Warning: New model type being bound to `RandomForestRegressor2` to avoid conflict with an existing
 name.
└ @ MLJModels ~/.julia/packages/MLJModels/p4SQ5/src/loading.jl:161
quote
    #= /home/expandingman/.julia/packages/MLJModels/p4SQ5/src/loading.jl:84 =#
    print("import MLJDecisionTreeInterface")
    import MLJDecisionTreeInterface
    println(" ✔")
    print("const RandomForestRegressor2 = MLJDecisionTreeInterface.RandomForestRegressor")
    const RandomForestRegressor2 = MLJDecisionTreeInterface.RandomForestRegressor
    println(" ✔")
    #= /home/expandingman/.julia/packages/MLJModels/p4SQ5/src/loading.jl:85 =#
    print("RandomForestRegressor2()")
    println(" ✔")
    RandomForestRegressor2()
end

This is problematic, because if this code block ever gets re-evaluated, which is extremely common with Revise, it will throw a warning about a const being re-defined. Is there any reason for this const? Seems to me this should be as simple as

using MLJDecisionTreeInterface: RandomForestRegressor

Note that I specified using instead of import so that users don't mistakenly load methods (though maybe this would be a feature? I guess I'm not totally decided on that)

[ExpandingMan]

Sorry, I only just realized that the conflict is coming from it defining a new name, not from overloading the const. Same question still applies.

[ablaom]

The const is just there to avoid a non-constant variable in global scope. Your remedy evidently does not work when a genuinely new name is desired (because of conflict with existing name, or because user explicitly requests this with name=...). Do you have a different suggestion?

Are you indeed encountering issues with Revise? If so, that's definitely a problem. I'm not sure I understand why Revise would cause a problem in this case (macro-expanded code) and yet not in the case of ordinary code const x=7 - but I am no expert and appreciate any guidance.

[ExpandingMan]

There are a couple of issues here, in part with Revise, though it's not quite what I was originally claiming.

In general, Julia is designed so that global statements such as import and using have no effect when re-executed. This has the virtue of making it possible to call scripts with e.g. include("scriptname.jl") as many times as you want without side effects.

MLJ breaks with this convention by creating a new alias from @load every time it is executed. While I don't think this is necessarily a bad idea in and of itself, I think in practice it is bad mainly because it's an unexpected way for code to behave. Packages such as Revise can also trigger new @load statements unexpectedly.

My suggestion would be to remove the logic that causes these types of side effects in @load. My guess as to why it exists in the first place would be because of models in different packages with like names. I'm not entirely sure what the best way to resolve this is, but I think ideally it would be something closely analogous to what needs to happen under normal circumstances if multiple modules export the same symbol. Perhaps force the user to qualify in these cases? For example MLJDecisionTreeInterface.DecisionTreeRegressor, providing a convenient alias? Or perhaps allow @load Model as Alias?

[ablaom]

In general, Julia is designed so that global statements such as import and using have no effect when re-executed. This has the virtue of making it possible to call scripts with e.g. include("scriptname.jl") as many times as you want without side effects.

MLJ breaks with this convention by creating a new alias from @load every time it is executed.

In the intended behaviour no new alias is constructed. And I have been unable to reproduce such behaviour; see code below, for example.

I don't doubt you have experienced some issue but can you please submit an example demonstrating new aliases being created so I can investigate?

julia> @load LinearRegressor pkg=GLM
[ Info: For silent loading, specify `verbosity=0`. 
import MLJGLMInterface ✔
const LinearRegressor = MLJGLMInterface.LinearRegressor ✔
LinearRegressor() ✔
LinearRegressor(
    fit_intercept = true,
    allowrankdeficient = false) @831

julia> @load LinearRegressor pkg=GLM
[ Info: For silent loading, specify `verbosity=0`. 
[ Info: Model code for LinearRegressor already loaded
(MLJGLMInterface.LinearRegressor)() ✔
LinearRegressor(
    fit_intercept = true,
    allowrankdeficient = false) @394

julia> @load LinearRegressor pkg=MultivariateStats
[ Info: For silent loading, specify `verbosity=0`. 
[ Info: Model code for LinearRegressor already loaded
┌ Warning: New model type being bound to `LinearRegressor2` to avoid conflict with an existing name. 
└ @ MLJModels ~/Dropbox/Julia7/MLJ/MLJModels/src/loading.jl:164
import MLJMultivariateStatsInterface ✔
const LinearRegressor2 = MLJMultivariateStatsInterface.LinearRegressor ✔
LinearRegressor2() ✔
LinearRegressor(
    bias = true) @847

julia> LinearRegressor()
LinearRegressor(
    fit_intercept = true,
    allowrankdeficient = false) @606

julia> LinearRegressor2()
LinearRegressor(
    bias = true) @443

julia> @load LinearRegressor pkg=MultivariateStats
[ Info: For silent loading, specify `verbosity=0`. 
[ Info: Model code for LinearRegressor already loaded
(MLJMultivariateStatsInterface.LinearRegressor)() ✔
LinearRegressor(
    bias = true) @533

julia> @load LinearRegressor pkg=MultivariateStats
[ Info: For silent loading, specify `verbosity=0`. 
[ Info: Model code for LinearRegressor already loaded
(MLJMultivariateStatsInterface.LinearRegressor)() ✔
LinearRegressor(
    bias = true) @169

[ExpandingMan]

Weird, I think this may be a bug, because I'm getting different behavior than you are showing here

julia> @load LinearRegressor pkg=GLM
[ Info: For silent loading, specify `verbosity=0`.
import MLJGLMInterface ✔
const LinearRegressor = MLJGLMInterface.LinearRegressor ✔
LinearRegressor() ✔
LinearRegressor)(
    fit_intercept = true,
    allowrankdeficient = false) @016

julia> @load LinearRegressor pkg=GLM
[ Info: For silent loading, specify `verbosity=0`.
┌ Warning: New model type being bound to `LinearRegressor2` to avoid conflict with an existing name.
└ @ MLJModels ~/.julia/packages/MLJModels/MizYQ/src/loading.jl:164
import MLJGLMInterface ✔
const LinearRegressor2 = MLJGLMInterface.LinearRegressor ✔
LinearRegressor2() ✔
LinearRegressor)(
    fit_intercept = true,
    allowrankdeficient = false) @567

This is on MLJ 0.15.0 on the Julia release-1.6 branch, though I have definitely see this behavior on 1.5.

Are you on MLJ master? I can't reproduce what you are showing, I always get the warning.

[ablaom]

I cannot reproduce on julia 1.6.

I suspect you are using an old MLJModels version, in which case we should bump the MLJ [compat] entry for MLJModels.

What version of MLJModels are you using?

I have:

(jl_70Gfas) pkg> st
Status `/private/var/folders/4n/gvbmlhdc8xj973001s6vdyw00000gq/T/jl_70Gfas/Project.toml`
  [caf8df21] MLJGLMInterface v0.1.2
  [d491faf4] MLJModels v0.13.2

[ablaom]

@ExpandingMan Trying hard, but still unable to reproduce your observations.

Can you please post a complete Manifest.toml. Thanks.

[ExpandingMan]

I can no longer reproduce either... I'm fairly sure something has changed though, I am now getting

[ Info: Model code for LinearRegressor already loaded

and I am quite certain I have never gotten that before.

[ExpandingMan]

I suspect another update inadvertently fixed this. I will close this now, and reopen with details if I see something analogous.

[ablaom]

Possibly a resolution of https://github.com/alan-turing-institute/MLJModels.jl/issues/353 (patch to appear soon) will resolve this too.