Respect Marked exception during model loading. #2574
Conversation
Ivanidzo4ka
requested review from
eerhardt,
TomFinley and
yaeldekel
and removed request for
eerhardt
Codecov Report
@@ Coverage Diff @@
## master #2574 +/- ##
=========================================
Coverage ? 71.5%
=========================================
Files ? 801
Lines ? 142036
Branches ? 16151
=========================================
Hits ? 101561
Misses ? 36003
Partials ? 4472
|
| catch (Exception ex) | ||
| { | ||
| if (ex.InnerException != null && ex.InnerException.IsMarked()) | ||
| throw Contracts.Except(ex, "Error during class instantiation"); |
There was a problem hiding this comment.
Does this actually add value? Why not just let the original exception be thrown? It will have the stacktrace of where it came from.
There was a problem hiding this comment.
Because of Invoke call. If anything happens inside it, we got TargetInvocationException and original exception would be inside InnerException.
There was a problem hiding this comment.
At least catch (TargetInvocationException ex) here then. #Resolved
| if (ex.InnerException != null && ex.InnerException.IsMarked()) | ||
| throw Contracts.Except(ex, "Error during class instantiation"); | ||
| else | ||
| throw ex; |
There was a problem hiding this comment.
Please never do this.
throw; is what you want.
What you are doing is cutting off the stack trace from the original exception, which contains valuable information when debugging. #Resolved
| @@ -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) | |||
There was a problem hiding this comment.
I like this much better. #Closed
| @@ -69,6 +69,7 @@ private static ITransformer Create(IHostEnvironment env, ModelLoadContext ctx) | |||
| var contractName = ctx.LoadString(); | |||
|
|
|||
| var composition = env.GetCompositionContainer(); | |||
| Contracts.CheckValue(composition,nameof(composition)); | |||
There was a problem hiding this comment.
Note that this is being deleted with #2569
There was a problem hiding this comment.
I understand.
I'm just trying to resolve this comment from TestCustomTrasformer :
we should have a common mechanism that will make sure this is 'our' exception thrown.
Only way I know how to do that is to check was exception marked or not.
Currently composition is null, and in next line it throws NullReference. Which is unmarked, and in order to make it marked, I need to run check on it.
| { | ||
| if (!ex.IsMarked()) | ||
| throw ex; |
There was a problem hiding this comment.
Again here - throw;. #Resolved
| @@ -69,9 +69,10 @@ public void TestCustomTransformer() | |||
| TestEstimatorCore(customEst, data); | |||
| Assert.True(false, "Cannot work without MEF injection"); | |||
| } | |||
| catch (Exception) | |||
| catch (Exception ex) | |||
There was a problem hiding this comment.
Can you change this to be the specific type of exception we expect?
And possibly adding a check for the message - since I'm sure it will just be InvalidOperationException.
ghost
locked as resolved and limited conversation to collaborators
address #2567