Closed tlienart closed 3 years ago
@tlienart Thanks indeed for these comments. I think it's a super ideas to have a complete working implementation of the API by a third party DummyModels package. And I do not have any objections to the format you suggest, although:
I am inclined nowadays to implement model traits manually. The convenience methods using eval
to automate this are actually more work to maintain and represent "hidden knowledge" that might put some off. However, the @mlj_model
is extremely convenient and I am committed to maintaining it.
I suggest we don't burden developers with a requirement that they conform to the template we create, just make it a suggestion. Ultimately the responsibility for maintenance is going to be theirs anyway, and some may find it easier to work directly with the requirements outlined in the docs than to conform to the template.
Ideally the DummyModels package should include a DummyDeterministicClassifier as this is the one that causes the most challenges, but anything is going to be better than nothing.
I think the newly created template repo MLJExampleInterface.jl addresses at least the gist of the issue raised here.
cc: @ablaom
While helping Yaqub & co with their interface and also thinking about #10, I realised that, currently, the way we suggest users write an interface is maybe not ideal. Unfortunately the examples we have are somewhat flawed:
EvoTrees
keeps their type and just extendsfit/predict
, author was happy to tightly integrate with MLJ, and have all models subtype MLJ types, this should not be expected (devs will usually not want to just subtype our type system, see #10)MLJLinearModels
in that case I just exported new model names for all models, same with MLJModelsSo basically we don't quite nail this. I think the interface should be a standalone thing which is as easy to maintain as possible and doesn't interfere with the rest of the package. The plus side from us is that if everyone does the same thing it might also make maintenance of the model registry smoother.
I'd like to suggest one way of doing this which could become the de-facto blueprint, this is somewhat inspired from how Yaqub wrote their interface. It's maybe not exactly what is required in #10 where devs would prefer not to have to redefine structs but I think that's only a minor nuisance.
Overview of the suggestion
Devs write a submodule
MLJInterface
inside their package which:@mlj_model
,fit
andpredict
calling the original corresponding functions from their package,A quirk of submodules and evaluation scopes makes it necessary to load this submodule in the package
__init__
function.Full working example
(I also suggest we register such an example in order to explicitly document the procedure and do so better than the current examples which are not quite representative IMO).
Package
In
FooModels/src/FooModels.jl
:Interface submodule
In
FooModels/src/MLJInterface.jl
:Tester script
Final notes
The above already works, the only thing is to make sure that when MLJ users try to use
FooRegressor
they don't have to callMLJInterface.FooRegressor
or something similar but I believe this is handled by the model registry (?).MLJInterface
we could just have the fully qualifiedFooModels.MLJInterface.FooRegressor
and let the registry infer the right short name