-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Let ImageLoadingTransformer dispose the last image it loads #5056
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
Conversation
@@ -1126,6 +1127,81 @@ public void TensorFlowTransformCifarSavedModel() | |||
} | |||
} | |||
|
|||
public class InMemoryImage | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same class as the one in ImageTests.cs?
They both seem to be in the same project. Can one of them be eliminated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they're the same class. But I wouldn't know how to share them across those tests without messing up the namespaces (one test is in Microsoft.ML.Tests and the other one in Microsoft.ML.Scenarios), so I just think it's better to keep them separate. Unless I add it to a general utils namespace, I'm not sure what that would be, or if that's desirable since this class is very specifical to the 2 tests I made anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply add a using Microsoft.ML.Tests in the this file and reference the class.
In reply to: 414190280 [](ancestors = 414190280)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that would make accessible anything from Microsoft.ML.Tests on the tensorflow test file, and that wouldn't be right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll use "using InMemoryImage = Microsoft.ML.Tests.ImageTests.InMemoryImage;" inside the tensorflow tests 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #4126.
By investigating issue #4126 I found out that
ImageResizingTransformer
disposed the last input Bitmap object it resized (i.e. only the last image object in the input dataset). When working with in-memory images (i.e. NOT loaded withImageLoaderTransformer
) this is inadequate, since theImageResizingTransformer
will dispose the last image in the dataset, and the user won't be able to use that image again. Particullarly, issue #4126 happened when running Cross Validation with a pipeline that included applyingImageResizingTransformer
to in memory images: when scoring the first model during cross validation, the last image of the input dataset got disposed, and when trying to score the second model, the image wasn't there, and a exception was thrown.The only reason I found as to why
ImageResizingTransformer
had this behavior, is becauseImageLoadingTransformer
actually disposes all the images it loads except for the last one (link). So assuming this is the only reason forImageResizingTransformer's
disposer, I simply moved the disposal mechanism of the last image toImageLoadingTransformer
.I added two tests that used to throw exceptions without the changes on this PR. And I also manually tested with the debugger's profiler that the last image got correctly disposed when using
ImageLoadingTransformer
.