Skip to content

Make sure RE is closed#47

Merged
joshmoore merged 2 commits intoome:masterfrom
dominikl:fix_info_leak
Oct 30, 2020
Merged

Make sure RE is closed#47
joshmoore merged 2 commits intoome:masterfrom
dominikl:fix_info_leak

Conversation

@dominikl
Copy link
Member

@dominikl dominikl commented Oct 7, 2020

The info command creates a RenderObject which loads a RenderingEngine but doesn't close it. When you run it on a large Plate you soon hit an OutOfMemory exception. This PR closes the RenderingEngine again.

The PR also adds a thumb command which generates rendering settings (if there aren't any yet) and creates the thumbnails. Without it one has to run first render info and then test --thumb.

@dominikl
Copy link
Member Author

dominikl commented Oct 8, 2020

Note: It makes the info command much slower, 2-3 times, unfortunately. But leaving the rendering engines open isn't really an alternative.

@dominikl
Copy link
Member Author

dominikl commented Oct 8, 2020

I'm running this currently in pilot-idr0093. It takes ages, and needs to be followed by another run of render test --thumb to create the thumbnails. This will take a long time too, because again it needs opening and closing rendering engines. Hence, I'll add another command 'thumb' which combines these two steps, so that a rendernig engine only has to be created once per image.

@jburel
Copy link
Member

jburel commented Oct 9, 2020

Note: It makes the info command much slower, 2-3 times, unfortunately. But leaving the rendering engines open isn't really an alternative.

I don't think starting a rendering engine to get the rendering setting info is the way to go when the settings exist.
it is an overkill

@dominikl
Copy link
Member Author

dominikl commented Oct 9, 2020

Could improve the RenderObject in another PR, add an option to load the settings without opening a rendering engine. But at least for now the PR makes sure the open rendering engine is closed again. Having the option to start a rendering engine is necessary too, in order to create rendering settings if there aren't any (I assume that's what it actually does).

@jburel
Copy link
Member

jburel commented Oct 9, 2020

When you don't have settings: yes, but when they exist it is not needed.
It is similar in a way to https://github.com/ome/omero-py/blob/master/src/omero/gateway/__init__.py#L8644

@jburel
Copy link
Member

jburel commented Oct 9, 2020

info and thumb commands now close the RE.
Since this PR is about closing RE, could you also check the tests?, e.g. assert_target_rdef

@dominikl
Copy link
Member Author

dominikl commented Oct 9, 2020

Sure. I'll also try to fix the RenderObject in this PR, so that it doesn't unnecessarily open a rendering engine.

@dominikl
Copy link
Member Author

dominikl commented Oct 30, 2020

I removed the last commit again. A separate thumb command as wells as @jburel 's suggestions should be part of a separate PR. So this PR is really only about closing the rendering engine, so that the PR can be merged and the leak fixed.

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

LGTM. I can imagine that something like the "--keep-going" flag that we have elsewhere (import.py) could be used to determine a throw or a ctx.err.

@joshmoore joshmoore merged commit a80e4e3 into ome:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants