Bad exception experience loading a model

[eerhardt]

See this PR review comment: https://github.com/dotnet/machinelearning/pull/1951/files#r244808064.

machinelearning/src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs

Lines 255 to 263 in 13b3339

	try
	{
	ModelLoadContext.LoadModel<TransformerChain<ITransformer>, SignatureLoadModel>(env, out var transformerChain, rep, LoaderSignature);
	return transformerChain;
	}
	catch
	{
	var chain = ModelFileUtils.LoadPipeline(env, stream, new MultiFileSource(null), extractInnerPipe: false);
	TransformerChain<ITransformer> transformChain = (chain as CompositeDataLoader).GetTransformer();

This is causing a real problem with the Custom Mapping Transformer work that I'm doing.

When a user tries loading a model that contains a Custom Mapping Transformer, but we can't find that extension/contract we throw an exception during ModelLoadContext.LoadModel above saying Can't find extension 'foo'.

However, this code here eats that exception, tries doing something else and then throws a terrible exception Repository doesn't contain entry DataLoaderModel\Model.key, which makes absolutely no sense to the user.

We should change this code such that the original exception is thrown if we can't load the Pipeline. Maybe we could start writing a flag into a "pipeline" model file to tell if it is supposed to be a Pipeline or not...? If we don't see that flag, then for sure the exception from ModelLoadContext.LoadModel above should be thrown.

@TomFinley @Ivanidzo4ka - Thoughts?

[eerhardt]

Side note: when fixing this, we should also fix this test:

machinelearning/test/Microsoft.ML.Tests/Transformers/CustomMappingTests.cs

Lines 67 to 75 in 174de1a

	try
	{
	TestEstimatorCore(customEst, data);
	Assert.True(false, "Cannot work without MEF injection");
	}
	catch (Exception)
	{
	// REVIEW: we should have a common mechanism that will make sure this is 'our' exception thrown.
	}

Which would have caught the problem if it was well written. This test should be ensuring the correct exception is thrown, not just any exception.

[TomFinley]

I actually ran into this myself. I spent about a half hour confirming , before realizing that something in our code stack was actually catching, then eating, the exception (in my case a null reference exception from some incorrect code I wrote), replacing it with this utterly unhelpful and incorrect message during model deserialization.

As you mentioned I had commented that doing this was a bad idea in principle, but I didn't realize at the time just how big of an impact that would have. I thought it was limited to a relatively limited scenario for backwards compatibility of some legacy models, so we could (regrettably) afford some ugliness, assuming it was actually explained why it was necessary. (Still not quite sure why it's needed?) If that same code is also being applied to newly serialized models, then that is a serious flaw for sure.