Skip to content
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

Add option to create new ThumbnailStore connection #129

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dominikl
Copy link
Member

Just adds an option to _prepareTB to use a new ThumbnailStore connection. If the cached one of the BlitzGateway is used then you can't call getThumbnail for multiple images in parallel.

Required by ome/omero-cli-render#33

@joshmoore
Copy link
Member

In general 👍 for the added functionality. The only thing that it would be good to consider is if there is any unification needed in terms of the API if we foresee ourselves adding something similar to other methods. cc: @will-moore

A few ideas:

  • drop "_ts"
  • if there are other similar arguments somewhere, match them
  • perhaps make this global state on the conn object? e.g. conn.cacheServices(False)

but probably could use some discussion before any more commits, @dominikl.

@@ -8395,7 +8401,7 @@ def _getProjectedThumbnail(self, size, pos):

# @setsessiongroup
def getThumbnail(self, size=(64, 64), z=None, t=None, direct=True,
rdefId=None):
rdefId=None, use_cached_ts=True):
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this if the default use_cached_ts is True.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite sure. So only def getThumbnail(self, size=(64, 64), z=None, t=None, direct=True, rdefId=None, use_cached_ts) then check with if not use_cached_ts?

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't surprise me that the same argument would need to be present on multiple methods for a value to be propagated through a call stack (if it's not stateful).

Copy link
Member

Choose a reason for hiding this comment

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

@dominikl
Copy link
Member Author

With the last commit you can set cacheServices to False on the Blitzgateway itself. Then calling methods like getThumbnailStore will use a new connection.

@joshmoore
Copy link
Member

>>> BG1 = omero.gateway.BlitzGateway(...secret..., secure=True)
>>> BG1.connect()
True

# Initially gives the same value
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>

# Can be modified to give different values
>>> BG1.cacheServices(False)
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c4a8>
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c518>

# Can be reverted back to the original behavior
# BUT: note that the returned value is the very first one!
>>> BG1.cacheServices(True)
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>

# Can be taught from the outset to give different values
>>> BG2 = omero.gateway.BlitzGateway(...secret..., secure=True, cache_services=False)
>>> BG2.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c470>
>>> BG2.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c400>

Functionality seems to work as expected. 👍 The only issue that I note is that when using the mutator (cacheServices(False)) the existing instances are still held on to. There might be a potential for someone to leave a service hanging around unclosed. Since the new behavior is not the default, this probably isn't an immediate concern. If we foresee that we might change this behavior and if that change would be breaking, then we might want to either fix it from the outset or at least clarify it in the docs.

@dominikl
Copy link
Member Author

dominikl commented Dec 2, 2019

Would it be useful to keep track of these ProxyObjectWrappers and add another method like clearUncachedServices() (or something like that) which can be used to close all connections which have been created while cacheServices was set to False?

@joshmoore
Copy link
Member

I'm not sure. As long as the docs are clear on who's responsible in each case. For example, I'm pretty sure we've hunted down the various "leaks" caused by not calling close on the cached services. It'd be a shame to introduce something similar. Ultimately, who owns the pointers?

@dominikl
Copy link
Member Author

dominikl commented Jan 8, 2020

--exclude
I'll exclude that for now. Yes, it's not a good idea the open up the possibility to create leaks. I think the previous way to only allow this for the thumbailstore exceptionally (commit
e0a5126 ) is better/safer although maybe a bit 'dirty'. Ok to revert back to this commit?

@will-moore
Copy link
Member

I ran into the need to NOT reuse rawPixelStore recently when multiple threads try to load pixel data simultaneously from napari - see https://forum.image.sc/t/napari-lazy-loading-from-omero/32292
So I think there will be more cases where this is needed.

@joshmoore
Copy link
Member

Happy to discuss. Technically a new method should mean this is a minor bump, so 5.6.0 is a good target. Would having cacheServices(False) clear existing services suffice to prevent a leak?

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.

4 participants