Skip to content

Respect Marked exception during model loading. #2574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/Microsoft.ML.Core/ComponentModel/ComponentCatalog.cs
Original file line number Diff line number Diff line change
@@ -185,16 +185,25 @@ internal LoadableClassInfo(LoadableClassAttributeBase attr, MethodInfo getter, C
internal object CreateInstanceCore(object[] ctorArgs)
{
Contracts.Assert(Utils.Size(ctorArgs) == CtorTypes.Length + ((RequireEnvironment) ? 1 : 0));

if (InstanceGetter != null)
try
{
Contracts.Assert(Utils.Size(ctorArgs) == 0);
return InstanceGetter.Invoke(null, null);
if (InstanceGetter != null)
{
Contracts.Assert(Utils.Size(ctorArgs) == 0);
return InstanceGetter.Invoke(null, null);
}
if (Constructor != null)
return Constructor.Invoke(ctorArgs);
if (CreateMethod != null)
return CreateMethod.Invoke(null, ctorArgs);
}
catch (TargetInvocationException ex)
{
if (ex.InnerException != null && ex.InnerException.IsMarked())
throw Contracts.Except(ex, "Error during class instantiation");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually add value? Why not just let the original exception be thrown? It will have the stacktrace of where it came from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of Invoke call. If anything happens inside it, we got TargetInvocationException and original exception would be inside InnerException.

Copy link
Member

@eerhardt eerhardt Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least catch (TargetInvocationException ex) here then. #Resolved

else
throw;
}
if (Constructor != null)
return Constructor.Invoke(ctorArgs);
if (CreateMethod != null)
return CreateMethod.Invoke(null, ctorArgs);
throw Contracts.Except("Can't instantiate class '{0}'", Type.Name);
}

4 changes: 3 additions & 1 deletion src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs
Original file line number Diff line number Diff line change
@@ -257,8 +257,10 @@ public static TransformerChain<ITransformer> LoadFrom(IHostEnvironment env, Stre
ModelLoadContext.LoadModel<TransformerChain<ITransformer>, SignatureLoadModel>(env, out var transformerChain, rep, LoaderSignature);
return transformerChain;
}
catch
catch (FormatException ex)
Copy link
Member

@eerhardt eerhardt Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this much better. #Closed

{
if (!ex.IsMarked())
throw;
var chain = ModelFileUtils.LoadPipeline(env, stream, new MultiFileSource(null), extractInnerPipe: false);
TransformerChain<ITransformer> transformChain = (chain as CompositeDataLoader).GetTransformer();
var predictor = ModelFileUtils.LoadPredictorOrNull(env, stream);
2 changes: 2 additions & 0 deletions src/Microsoft.ML.Transforms/LambdaTransform.cs
Original file line number Diff line number Diff line change
@@ -69,6 +69,8 @@ private static ITransformer Create(IHostEnvironment env, ModelLoadContext ctx)
var contractName = ctx.LoadString();

var composition = env.GetCompositionContainer();
if (composition == null)
throw Contracts.Except("Unable to get the MEF composition container");
ITransformer transformer = composition.GetExportedValue<ITransformer>(contractName);
return transformer;
}
5 changes: 3 additions & 2 deletions test/Microsoft.ML.Tests/Transformers/CustomMappingTests.cs
Original file line number Diff line number Diff line change
@@ -69,9 +69,10 @@ public void TestCustomTransformer()
TestEstimatorCore(customEst, data);
Assert.True(false, "Cannot work without MEF injection");
}
catch (Exception)
catch (InvalidOperationException ex)
{
// REVIEW: we should have a common mechanism that will make sure this is 'our' exception thrown.
if (!ex.IsMarked())
throw;
}
ML.CompositionContainer = new CompositionContainer(new TypeCatalog(typeof(MyLambda)));
TestEstimatorCore(customEst, data);