tlambert03 / napari-omero

Browse your OMERO database from within napari.
GNU General Public License v2.0
33 stars 10 forks source link

Add groups and users toggles #46

Open tlambert03 opened 1 year ago

tlambert03 commented 1 year ago

closes #31 includes #45

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 35.84906% with 68 lines in your changes missing coverage. Please review.

Project coverage is 33.48%. Comparing base (cbcd287) to head (5f1b48f). Report is 2 commits behind head on main.

Files Patch % Lines
src/napari_omero/widgets/main.py 38.27% 50 Missing :warning:
src/napari_omero/widgets/tree_model.py 29.16% 17 Missing :warning:
src/napari_omero/widgets/thumb_grid.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #46 +/- ## ========================================== + Coverage 33.41% 33.48% +0.06% ========================================== Files 13 13 Lines 772 866 +94 ========================================== + Hits 258 290 +32 - Misses 514 576 +62 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

will-moore commented 1 year ago

Just giving this a test...

I don't see that the User list is updated when you switch groups - It needs to list the members of the currently-selected Group.

When the list of projects loads initially (on login) I see the list of Projects & Datasets that belongs to the current user is appended below those for "All" users (see screenshot below). When you switch user this resolves fine.

Screenshot 2023-04-18 at 12 19 04

I think it's safer not to use conn.setGroupForSession() to filter by group. In the webclient we switched to using conn.SERVICE_OPTS.setOmeroGroup(group) and this works better.

--- a/src/napari_omero/widgets/tree_model.py
+++ b/src/napari_omero/widgets/tree_model.py
@@ -100,8 +100,9 @@ class OMEROTreeModel(QStandardItemModel):
         opts = {"order_by": "obj.name"}
         if owner is not None:
             opts["owner"] = owner
-        if group is not None:
-            self.gateway.conn.setGroupForSession(group)
+        if group is None:
+            group = -1
+        self.gateway.conn.SERVICE_OPTS.setOmeroGroup(group)
         return itertools.chain(
             self.gateway.getObjects("Project", opts=opts),
             self.gateway.getObjects("Dataset", opts={**opts, "orphaned": True}),
tlambert03 commented 1 year ago

Thanks for your input @will-moore ! Very helpful.

I did look through some of your web portal code after writing this and realized a couple of these better patterns (with service opts). Will change!

tlambert03 commented 1 year ago

ok getting closer. If @will-moore or @romainGuiet wants to give it another try, feedback appreciated (@will-moore, particularly keen to know if you see any more bad patterns with the OMERO API)

will-moore commented 1 year ago

This looks good for the switching of users and groups.

However, I'm seeing some issues with Thumbnails when you try to view a different user's data in a group that isn't your default group (when you switch group AND user)...

I haven't clearly established which of these errors is the original cause and which might be an effect:

    serverStackTrace = ome.conditions.InternalException: Hibernate session is not re-entrant.
Either you have two threads operating on the same stateful object (don't do this)
 or you have a recursive call (recurse on the unwrapped object). 
  message =  Wrapped Exception: (org.springframework.transaction.TransactionSystemException):
Could not roll back JDBC transaction; nested exception is java.sql.SQLException: connection handle already closed

I think that if you don't look at your own thumbnails first, then you don't see this error. E.g. if you log-in then immediately change group and user, you can view their thumbnails without a problem.

After looking at this a while, it seems that the thumbnailstore is getting re-used with different permissions. Unfortunately the BlitzGateway doesn't provide a very nice way to force clean-up. Most usage of BlitzGateway is in the webclient, where each http request creates a new BlitzGateway from an existing OMERO session, so we don't have to worry about switching context and old state remaining.

This is what I tried, and it seems to help. I also switched to using a single call to conn.getThumbnailSet() which should be more efficient - although it didn't seem to make any difference with the errors above. NB: I guess with very large Datasets this could start to fail - not tested yet.

--- a/src/napari_omero/widgets/thumb_grid.py
+++ b/src/napari_omero/widgets/thumb_grid.py
@@ -6,6 +6,7 @@ from qtpy.QtWidgets import QListWidget, QListWidgetItem

 from .gateway import QGateWay
 from .tree_model import OMEROTreeItem
+from omero.gateway import ProxyObjectWrapper

 THUMBSIZE = 96

@@ -63,9 +64,14 @@ class ThumbGrid(QListWidget):
         def yield_thumbs(conn):
             self.clear()
             self._item_map.clear()
-            for img in item.wrapper.listChildren():
-                for byte in conn.getThumbnailSet([img.getId()], THUMBSIZE).values():
-                    yield byte, img
+            images = list(item.wrapper.listChildren())
+            thumbs = conn.getThumbnailSet([img.id for img in images], THUMBSIZE)
+            # clean up thumbnail store
+            conn._proxies['thumbs'] = ProxyObjectWrapper(conn, 'createThumbnailStore')
+            for img in images:
+                yield thumbs[img.id], img