-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Description
Consider this attribute class:
public sealed class ComponentKindAttribute : Attribute |
It looks harmless enough, but kind of isn't. It exists, as far as I can tell, to give some sort of "name" to various types of entry-point nodes. These mostly serve to decorate specific IComponentFactory
subinterfaces. However, because we need to attach this attribute to a definite type, we have the following implication: that we've littered our codebase with a bunch of interfaces as subinterfaces of IComponentFactory
, and these exist only so we can attach attributes to them. To take just one example:
machinelearning/src/Microsoft.ML.Data/Utils/LossFunctions.cs
Lines 84 to 87 in 7e21afa
[TlcModule.ComponentKind("SDCAClassificationLossFunction")] | |
public interface ISupportSdcaClassificationLossFactory : IComponentFactory<ISupportSdcaClassificationLoss> | |
{ | |
} |
That's fairly obnoxious. From an API user's perspective, this represents a completely pointless complication -- all an API user really wants to do is be able to set a loss function. But because we have this other "entry point" thing we also have to work through this generic way to create subcomponents (this IComponentFactory
thing, which is subideal but possibly OK), but then because we also wanted to attach an attribute to this "type" of set of component factories which we did through attributes on the type, which required creating this empty interface that otherwise has no reason to exist -- etc., etc. Anyway, we see that things have piled up.
What I might like instead is the following:
-
In all cases, never exposing the empty
IComponentFactory
derived empty interfaces that exist just for the sake of this attribute never be publicly accessible. -
Ideally have some alternative for entry-points that does not intersect with the public surface of the API in such a visible way. I am having difficulty with suggesting a specific course here, since I am having difficulty finding anyone that knows why this attribute was introduced in the first place, or how it is used in entry-points. (Though, it seems to be used.)
-
Where possibly and reasonable, replace the public surface of the API of
IComponentFactory<Foo>
withFoo
directly. As seen above for the case of loss functions, there is no reason not to do this. Indeed, in some places we have done this, which is good:ISupportSdcaClassificationLoss loss, Yet there are too many places in the code where we have not, e.g., here:
ISupportClassificationLossFactory loss = null,
or here:
public ISupportClassificationLossFactory LossFunction = new LogLossFactory(); |
The reasons for this are somewhat historical, in that we had the attitude for a while that the statically typed API would be the public face, but if the dynamic API is to be the public face then it should be a little less terrible than it is now.