Need to add API breaking change definition and enforce it

[shauheen]

With version 1.0.0 release we need to formally adopt the API breaking change definitions similar to CoreFx and here. We also need to add a test to run APICompat tool to enforce these rules. @eerhardt @TomFinley @glebuk

[TomFinley]

OK, so, as discussed elsewhere with @eerhardt and @shauheen we thought we might consider variances in the rules. Just to kick off that discussion, there are two variances of the "rules" that I had at the top of my mind I thought I'd start with. I'll start with the more controversial one...

Default Parameter Values Change for IEstimator Configuration?

The corefx rules on default values are quite clear that changing defaults is disallowed, and I strongly believe this is an essential policy to maintain:

Changing the default value for a property, field or parameter (either via an overload or default value)

However, in ML, good defaults are important; even people that know better sometimes just trust defaults, and hope for a good model. So: having good defaults has always been fertile ground to help people get a good experience quickly.

The trouble is, what are good defaults is sometimes far from obvious. The defaults for hyperparameters are initially SWAGs based on no real evidence in particular -- just guesses as to what might be useful. Following months or sometimes even years of practical experience across many problems, we reach a conclusion that a default may be worth changing. I think internally the associated toolkit's popularity was due at least in part to the fact that we had data scientists doing that hard work of deciding what were sensible values.

However even many years later sometimes we still have somewhat troublesome defaults running around:

machinelearning/src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LbfgsPredictorBase.cs

Line 130 in c587f82

public const float OptimizationTolerance = 1e-7f;

Isn't that tolerance just a little tight? Couldn't we get away with something smaller? Maybe!

At the same time, the CoreFX prohibition on changing default values exists for very excellent reasons. So I'd like a very specific, insular variance to that rule under the following constraints:

1. only when we are creating or configuring an IEstimator instance (e.g., the default values that appear in the MLContext extension methods to create an IEstimator),

2. we never change the fundamental .NET types of that IEstimator instance by changing this default, either as a return value, or as part of the data pipeline,

3. we should not change the shape of schema of the dataview, but we might (reluctantly) change the schema itself in a way that is unlikely to have lasting consequences.

4. As a possible exception to the shape constraint, we might welcome a different default that provides additional annotations that were not there before, but we would not change a default that would remove an annotation.

Some examples...

Let's talk about changes to IEstimator default parameterization, and whether they'd be allowed or disallowed.

• Allowed. Changing one of the parameters that controls runtime behavior with no consequences to the output schema of the ITransformer. Perhaps an the optimization tolerance default value for LBFGS methods is set too low, or needlessly high. Perhaps the default number of iterations should change.

• Allowed but reluctantly. This is an example of point 3. Suppose we decide that the number of default bits for hash based featurization should go up or down. The "shape" of the schema hasn't changed one bit, but the schema itself has changed (in the sense that the output key type will have a different count, which has consequences downstream).

• Disallowed. This is an example of point 2. Consider this (now outdated) method. Let's imagine we wanted to change that mode to be one of the different sorts of normalization. This would not be allowed, because then reasonable code that would try to extract the normalization parameters by casting to the right form would start to fail.

  machinelearning/src/Microsoft.ML.Transforms/NormalizerCatalog.cs

  Lines 26 to 28 in 5f9be36

  	public static NormalizingEstimator Normalize(this TransformsCatalog catalog,
  	string outputColumnName, string inputColumnName = null,
  	NormalizingEstimator.NormalizationMode mode = NormalizingEstimator.NormalizationMode.MinMax)

  If we could change it, then perfectly reassonable existing code which fits the estimator, casts the resulting column function to an appropriate type. (Now, we happened to have avoided this problem altogether by just having separate methods for each of the types rather than have an enum, but if we hadn't, then this would have been an example of an "unchangable default".)

  Also: let's imagine that we change a default that does change the schema shape, e.g., we decide that conversion should convert to double by default instead of float.

Better More Specific Exceptions

Second proposal for variance, probably less controversial...

Consider these rules around exceptions.

So, there's a lot of code in ML.NET. We have been, I think, pretty good about providing actionable exception messages, but I know there's plenty that was missed. Maybe some data leads to an array index out of bounds. Maybe there's a null reference exception buried in some obscure place due to a bad parameter being passed in. I'd like future maintainers to feel free to detect bad inputs and introduce meaningful actionable exception messages to the user.

[eerhardt]

One clarifying question:

2. we never not change the fundamental .NET types of that IEstimator instance by changing this default, either as a return value, or as part of the data pipeline,

I believe the double negative was not intended, was it? It should be we never change...

---

The issue with changing a parameter's default value in .NET is that the default value is always burned into the calling code, not in the method that defines the parameter and its default.

So that means if you have ML.NET version 1.0 with a default parameter value foo, and a consuming program compiled against 1.0 will get the default value foo.

If then, in ML.NET 1.1 we change the default value to bar, but don't rebuild the consuming program (or more likely some other library that references ML.NET). When the program runs against ML.NET 1.1, it will still pass the value foo into the method. The consuming program would need to be recompiled against 1.1 to pick up the new default value.

---

For exceptions, I think undocumented exceptions (like null refs and obscure ArgumentOutOfRange) for sure should be fixed. Those are almost always are just bugs.

I wonder how much of those referenced rules are specific to "documented exceptions" vs. accidental/bug exceptions.

[TomFinley]

Hi @eerhardt thanks for replying.

I believe the double negative was not intended, was it? It should be we never change...

You are correct, sorry about that. Edited to remove the "not."

The issue with changing a parameter's default value in .NET is that the default value is always burned into the calling code, not in the method that defines the parameter and its default.

Actually that does not strike me as that much of a problem. If someone did reference ML.NET v1.0 and had compiled code, it might not be such a bad thing if they were happy with the old defaults to continue to function with them. That is, that's probably the safest course. So I'm not sure I'm too concerned about that, if someone references something with default number of iterations 1, and we change that default to 10 per #2305 hypothetically (more ideally, with evidence that doesn't appear to have been sketched in MS Paint), is it so bad if the older working compiled code continues to use 1, even if a recompilation or new projects might benefit from the newer "wisdom" of 10?

I feel also like restricting it to estimators specifically might give us some leeway. I think people are pretty unforgiving about existing models (e.g., ITransformers) having different behavior, but the training code, I kind of feel like there's a bit more wiggle room to consider ourselves to have some flexibility.

For exceptions, I think undocumented exceptions (like null refs and obscure ArgumentOutOfRange) for sure should be fixed. Those are almost always are just bugs.
I wonder how much of those referenced rules are specific to "documented exceptions" vs. accidental/bug exceptions.

Sounds good. I am not altogether certain we ever document the type of exception, though we do sometimes I am pretty sure mention we will mention an exception. The cases that I am talking about, if I look back on them, I don't think we ever really identify the type of exception.

So from your perspective, it sounds like, if we have some user facing code A that calls some other code B, and B throws the exception, we're free to add exception checking code in A, because the fact that A called a misconfigured B on account of bad input to A, could be treated as a bug in A, and so we're free to throw a different exception? Or have I gone a bit too broad with my interpretation?

On the subject of bugs... a more controversial point, there's also situations where something would "work" (in the sense of not producing an error) but produce nonsense or something not useful (e..g, a linear classification trainer diverges and produces a model with infinities in some coefficients), that we could potentially catch and so save people some grief.

Just clarifying where we're really at. 😄