Notice: In order to edit this ticket you need to be either: a Product Owner, The owner or the reporter of the ticket, or, in case of a Task not yet assigned, a team_member"

Task #11532 (accepted)

Opened 6 years ago

Last modified 4 years ago

Projected image: pixelsService.copyAndResizeImage() workaround

Reported by: jburel Owned by: mtbcarroll
Priority: critical Milestone: Unscheduled
Component: Services Version: n.a.
Keywords: graph Cc: fs@…, ux@…, nikolaus.ehrenfeuchter@…
Resources: n.a. Referenced By: n.a.
References: n.a. Remaining Time: n.a.
Sprint: n.a.

Description (last modified by mtbcarroll)

Not possible to move source image/projected image due to channels linkage.

Current Workaround: Delete the projections manually or ask the other user to if not possible.

Until we have deep copy on demand (!), for new projections it could be good if pixelsService.copyAndResizeImage() did *not* share instrument, objective settings, etc. between images.

Change History (47)

comment:1 Changed 6 years ago by mtbcarroll

Is it clear what should be done about this? "Everything is in exactly one group" has all kinds of interesting linkage-caused implications.

comment:2 Changed 6 years ago by jamoore

@mtbc: not really. At the moment the projected image is in a different MIF, so to speak, though it re-uses some internal objects. So options include:

  1. make them part of the same MIF
  2. make them *not* re-use the same objects
  3. add extra handling for finding and removing the projected images on chgrp/delete
  4. like 3, but leave them orphaned.
  5. ???

comment:3 Changed 6 years ago by jburel

  • Milestone changed from 5.0.0-beta2 to 5.0.0-beta3

graph problem moved Beta3

comment:6 Changed 5 years ago by jburel

  • Cc nikolaus.ehrenfeuchter@… added

comment:7 Changed 5 years ago by jburel

  • Milestone changed from 5.1.0 to 5.1.0-m4

comment:8 Changed 5 years ago by mtbcarroll

The core problem is that images owned by different users have some model objects in common -- probably instrument, objective settings -- and those objects may be in only one group? We won't be creating a copy of the shared objects at chgrp time for 5.1.0. A difficulty is deciding which image is the original: without special ID-comparing logic, we'd not want the owner of the projection to move their image and cause the original to be deleted. Of course, it may be that we can have the projection script not re-use the objects, and OMERO does tend to be less permissive than the OME core object model, but people will already have legacy projections in their systems anyway. Not sure if we ought to try to fix this soon or simply bring it into a larger discussion of the way forward permissions-and-model-wise.

comment:9 Changed 5 years ago by jburel

Problem is not only related to projected image as discussed with Mark
Any "derive" image will lead to similar error e.g. images from ROis.

comment:10 Changed 5 years ago by jamoore

  • Owner set to pwalczysko

Petr: there's some question if the state of "move projected image" is sufficiently good for you at the moment. If yes, then we can push this to "5.x". Otherwise, someone can you let us know the steps to badness.

comment:11 Changed 5 years ago by pwalczysko

Yes, atm., I am kinda happy for it to move to 5x. Steps to badness:

  • create a Projection
  • move it somewhere where you forgot the place of
  • try to move the original image -> move fails, no explanation why, no idea where the projection is anyway -> badness

comment:12 Changed 5 years ago by mtbcarroll

If it would help clients to handle this, the container service could offer a getImagesBySplitInstruments or somesuch.

Last edited 5 years ago by mtbcarroll (previous) (diff)

comment:13 Changed 5 years ago by wmoore

I would like it if pixelsService.copyAndResizeImage() created new objects in the DB instead of linking the new image to existing objects.
That would solve this issue for new projected images, channel offsets script etc, although if we still wanted to fix existing images then we'd also need another approach.

comment:14 Changed 5 years ago by jamoore

wmoore: the DB upgrade could possibly handle existing images.

comment:15 Changed 5 years ago by jamoore

Referencing ticket #11752 has changed sprint.

comment:16 Changed 5 years ago by jamoore

Referencing ticket #11752 has changed sprint.

comment:17 Changed 5 years ago by jamoore

Are we attempting anything here?

comment:18 Changed 5 years ago by pwalczysko

  • Owner pwalczysko deleted

Not me. The satatus to my knowledge is as reported by me couple of comments above.

comment:19 Changed 5 years ago by pwalczysko

(I guess this is not something we will attempt for 5.1.0)

comment:20 Changed 5 years ago by jamoore

  • Milestone changed from 5.1.0 to 5.1.1
  • Owner set to mtbcarroll
  • Priority changed from blocker to critical

Think this will have to go along with the "Graphs clean" (roughly with "Permissions" and "Export", followed by "Deep Copy").

Timing to be discussed.

comment:21 Changed 5 years ago by mtbcarroll

Naturally, timing depends on the chosen solution. For 5.1.1 we can at least provide some kind of getImagesBySplitObjects (looks at instruments, channels, objective settings) that is like checking if the images are in the same fileset, though ownership within the set may vary. For anything more like deep copy, even 5.1.x is optimistic, especially if we also have to duplicate annotations on the instrument, etc. You're right, definitely needs discussion.

comment:22 Changed 5 years ago by wmoore

Just found another bug associated with this issue:

  • I create a share and put image into it.
  • Other members of the share can view image OK etc.
  • Then I create a new image ('Channel Offset script OR Projection etc)
  • Now, other members of the share try to view the image that I previously added to the share:
Traceback:
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/django/core/handlers/base.py" in get_response
  114.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/wmoore/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/decorators.py" in wrapped
  469.             retval = f(request, *args, **kwargs)
File "/Users/wmoore/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/decorators.py" in wrapper
  519.             context = f(request, *args, **kwargs)
File "/Users/wmoore/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/webclient/views.py" in load_metadata_preview
  967.     rdefId = manager.image.getRenderingDefId()
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero/gateway/__init__.py" in wrapped
  6695.                 if not self._prepareRenderingEngine() \
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero/gateway/__init__.py" in _prepareRenderingEngine
  6920.                 self._re = self._prepareRE(rdid=rdid)
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero/gateway/__init__.py" in _prepareRE
  6889.         re.lookupPixels(pid, ctx)
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero/gateway/__init__.py" in __call__
  4120.             return self.handle_exception(e, *args, **kwargs)
File "/Users/wmoore/Desktop/OMERO/components/tools/OmeroWeb/omeroweb/webclient/webclient_gateway.py" in handle_exception
  2069.                 e, *args, **kwargs)
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero/gateway/__init__.py" in __call__
  4117.             return self.f(*args, **kwargs)
File "/Users/wmoore/Desktop/OMERO/dist/lib/python/omero_api_RenderingEngine_ice.py" in lookupPixels
  337.             return _M_omero.api.RenderingEngine._op_lookupPixels.invoke(self, ((pixelsId, ), _ctx))

Exception Type: SecurityViolation at /webclient/metadata_preview/image/8102/57579/
Exception Value: exception ::omero::SecurityViolation
{
    serverStackTrace = ome.conditions.SecurityViolation: ome.model.core.Channel:Id_8813 not contained in share
	at ome.security.sharing.SharingACLVoter.throwLoadViolation(SharingACLVoter.java:79)
	at ome.security.CompositeACLVoter.throwLoadViolation(CompositeACLVoter.java:92)
	at ome.security.ACLEventListener.onPostLoad(ACLEventListener.java:104)
	at org.hibernate.engine.TwoPhaseLoad.initializeEntity(TwoPhaseLoad.java:250)
	at org.hibernate.loader.Loader.initializeEntitiesAndCollections(Loader.java:898)
	at org.hibernate.loader.Loader.doQuery(Loader.java:773)

comment:23 Changed 5 years ago by mtbcarroll

This ticket is largely superseded by https://trello.com/c/muXgN0XR/213-deep-copy but is there some other ticket regarding the other share permissions issues we've been seeing that covers Will's latest?

comment:24 Changed 5 years ago by mtbcarroll

  • Description modified (diff)
  • Milestone changed from 5.1.1 to 5.1.2
  • Owner mtbcarroll deleted
  • Summary changed from Projected image. to Projected image: pixelsService.copyAndResizeImage() workaround

Will move deep copy issues to the Trello card: this ticket becomes devoted to the API behavior workaround.

comment:25 Changed 5 years ago by wmoore

As discussed with Mark, "deep copy" is a much bigger issue than the dual linking of new images to Channels, Instrument etc caused by pixelsService.copyAndResizeImage().
So lets keep a ticket for that smaller issue.
This issue is also a problem for shares: New ticket for that today:
https://trac.openmicroscopy.org.uk/ome/ticket/12832

comment:26 Changed 5 years ago by wmoore

Another use case that is broken by this bug - Can't project an image belonging to another user (read-ann group) #12806

comment:28 Changed 4 years ago by jamoore

  • Milestone changed from 5.1.2 to 5.1.3

comment:29 Changed 4 years ago by jamoore

  • Milestone changed from 5.1.4 to OMERO-5.1.4

Splitting 5.1.4 due to milestone decoupling

comment:30 Changed 4 years ago by jamoore

Note: it might suffice for 5.1.4 to simply comment out the re-use of instrument etc. See https://github.com/openmicroscopy/openmicroscopy/blob/v5.1.3/components/server/src/ome/logic/PixelsImpl.java#L238

comment:31 Changed 4 years ago by sbesson

  • Milestone changed from OMERO-5.1.4 to OMERO-5.2.0
  • Owner set to mtbcarroll

comment:32 Changed 4 years ago by mtbcarroll

  • Milestone changed from OMERO-5.2.0 to 5.x

If OMERO 5.2.0 code is to be largely open in PRs by mid-October then, with my preparing for the BioImage Informatics Conference, the acquisition object duplication for this ticket certainly isn't happening. If anyone wants to try getting some quick fix in in the meantime, feel free to steal this ticket, otherwise I'll still keep it on my priority list: happy to see it in 5.2.1 once that milestone exists.

Last edited 4 years ago by mtbcarroll (previous) (diff)

comment:33 Changed 4 years ago by mtbcarroll

  • Milestone changed from 5.x to OMERO-5.2.1

Fixing this actually fits in with ROI work.

comment:34 Changed 4 years ago by mtbcarroll

So, let's say there's a deep-copy graph request that can be submitted from client-side scripts. How to submit such requests from inside the server itself, i.e. from PixelsImpl? Have we any simple examples of this?

comment:35 Changed 4 years ago by jamoore

I think this best example of this is Deletion.java. i.e. encapsulate the logic in the server/ component so it can be used internally. If that proves too costly (in terms of refactoring), the next option would be to write a DuplicateMessage and put it on the event bus.

comment:36 Changed 4 years ago by mtbcarroll

  • Status changed from new to accepted

comment:37 Changed 4 years ago by mtbcarroll

Thank you! The more I pursue the first option, the more I think it's best to choose the latter.

comment:38 Changed 4 years ago by mtbcarroll

  • Version 4.4.8 deleted

The DuplicateMessage approach works well, except for that the duplication happens in a separate transaction because a different thread is needed to avoid blocking message dispatch.

However, it helps us to fix PixelsImpl rather less than I'd originally hoped: pixels need an image and channels need a pixels so they have to be duplicated attached to other things, and they also link via other model objects to detectors and instruments and stuff, so if any of those are to be duplicated then the whole set should probably be in the same operation, yet the pixels service offers ability to mess with the pixels object and its channels and whatnot, and to create extra pixels on the original image, so a whole subgraph duplicate isn't what's really wanted.

To avoid multiply duplicating subgraph fragments through reaching them via required properties from different objects that do need duplicating, it might actually be easier to duplicate the whole lot then delete or graft as appropriate. (For instance, to delete channels not in the provided channelList.) However, that requires addition of a DeleteMessage and yet more transactions (I assume we can't move PixelsImpl to Blitz), and pretty much a reimplementation of the pixels service.

I'm thus tempted to push this again, I'm afraid; I hadn't realized how awkwardly featureful the pixels service is. I wonder how well our integration testing covers those features.

Last edited 4 years ago by mtbcarroll (previous) (diff)

comment:39 Changed 4 years ago by jamoore

Happy both to discuss possible workarounds to these issues as well as to let someone else specify the priority for the changes.

comment:40 Changed 4 years ago by jburel

  • Milestone changed from OMERO-5.2.1 to OMERO-5.2.2

Milestone OMERO-5.2.1 deleted

comment:41 Changed 4 years ago by jburel

  • Milestone changed from OMERO-5.2.2 to OMERO-5.2.1

Milestone OMERO-5.2.2 deleted

comment:42 Changed 4 years ago by mtbcarroll

  • Milestone changed from OMERO-5.2.2 to 5.x

Pushing again, I'm afraid. I won't forget about this, but the model change work has to take priority at present.

comment:43 Changed 4 years ago by mtbcarroll

I hadn't realized how awkwardly featureful the pixels service is.

One question is how featureful it needs to be. If it requires major work for this bug, perhaps it can be made simpler without anybody minding.

comment:44 Changed 4 years ago by jamoore

  • Milestone changed from 5.x to Unscheduled

comment:45 Changed 4 years ago by mtbcarroll

  • Keywords graph added

comment:46 Changed 4 years ago by mtbcarroll

Also see #12580.

comment:47 Changed 4 years ago by mtbcarroll

Note #1217: projection should have different sha1.

Last edited 4 years ago by mtbcarroll (previous) (diff)
Note: See TracTickets for help on using tickets. You may also have a look at Agilo extensions to the ticket.

1.3.13-PRO © 2008-2011 Agilo Software all rights reserved (this page was served in: 0.439473 sec.)

We're Hiring!