allow creation of thumbnails in all-groups context #5207

mtbc · 2017-03-29T11:29:17Z

What this PR does

Adjusts the heart of the server's thumbnail generation so that in a omero.group: -1 context thumbnails can still be generated, by grouping the pixels objects by group and creating their thumbnails in the corresponding group context.

Testing this PR

Experiment with the thumbnail service to ensure that this PR does not cause any regressions.

Check if the new https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-java/lastCompletedBuild/testngreports/integration/ThumbnailStoreTest/testGetThumbnailsMultipleGroups/ test passes and looks to relate to what this PR does.

Related reading

https://trello.com/c/bJqd3SkJ/39-getthumbnailbylongestsideset-doesn-t-accept-ctx

no suitable context for creating new rendering settings

@jburel

suggested by @jburel

This reverts commit 12e38a6. It is to be replaced with an alternative fix.

triggers an issue noticed by @aleksandra-tarkowska

fix for https://trello.com/c/bJqd3SkJ/

will-moore · 2017-03-30T12:57:56Z

Testing locally with my branch based on @aleksandra-tarkowska's work seems to work fine.
I can browse thumbnails in webclient when they're not in my default group (using omero.group: -1).
I'll open my PR again to allow testing in web #5192

will-moore · 2017-03-30T13:08:36Z

After "Save to All" I am getting failures in getThumbnailSet() but not for getThumbnail(). Doesn't appear to be group-specific (same behaviour in default group vv other group, always using group -1). Need to investigate more...

mtbc · 2017-03-30T15:11:54Z

Hmmm. I tried switching testGetThumbnailsMultipleGroups to use getThumbnailSet and also to try a non-standard 72×72 thumbnail size and that still passes for me locally. I wonder if we can find a simple way to reconstruct the error that you are observing. Also, I wonder if it is a regression.

will-moore · 2017-03-30T15:47:12Z

So, after I do

settingsService.applySettingsToSet(pid, "Image", [56686])

I get an exception for

tb.getThumbnailByLongestSideSet(rint(96), [pid, pid2])

Exception!!! Traceback (most recent call last):
  File "/Users/wmoore/Desktop/OMERO/openmicroscopy/dist/lib/python/omero/gateway/__init__.py", line 4454, in getThumbnailSet
    rint(max_size), list(_temp), ctx)
  File "/Users/wmoore/Desktop/OMERO/openmicroscopy/dist/lib/python/omero/gateway/__init__.py", line 4514, in __call__
    return self.handle_exception(e, *args, **kwargs)
  File "/Users/wmoore/Desktop/OMERO/openmicroscopy/dist/lib/python/omeroweb/webclient/webclient_gateway.py", line 2025, in handle_exception
    e, *args, **kwargs)
  File "/Users/wmoore/Desktop/OMERO/openmicroscopy/dist/lib/python/omero/gateway/__init__.py", line 4511, in __call__
    return self.f(*args, **kwargs)
  File "/Users/wmoore/Desktop/OMERO/openmicroscopy/dist/lib/python/omero_api_ThumbnailStore_ice.py", line 650, in getThumbnailByLongestSideSet
    return _M_omero.api.ThumbnailStore._op_getThumbnailByLongestSideSet.invoke(self, ((size, pixelsIds), _ctx))
SecurityViolation: exception ::omero::SecurityViolation
{
    serverStackTrace = ome.conditions.SecurityViolation: Group is ------. Cannot link to object: ome.model.core.Pixels:Id_39754
	at ome.security.basic.OmeroInterceptor.throwIfNotGranted(OmeroInterceptor.java:1179)
	at ome.security.basic.OmeroInterceptor.evaluateLinkages(OmeroInterceptor.java:585)

but this is fixed AFTER I get a single thumbnail for that image (pid2).

will-moore · 2017-03-30T21:25:37Z

The error at #5207 (comment) was using a combination of webclient and python API. I'll try and make a self-contained test.

atarkowska · 2017-03-30T22:40:15Z

@mtbc do you expect https://github.com/openmicroscopy/openmicroscopy/pull/5158/files#diff-e4feecea30b2ee097639cbb7c9171fedR4438 to work now?

will-moore · 2017-03-30T22:53:26Z

// This works to start with:
tb.getThumbnailByLongestSideSet(rint(96), [pid1, pid2], {'omero.group': '-1'})

// But after doing this:
ss.applySettingsToSet(pid1, "Image", [imageId2])

// This now fails with the Exception above
tb.getThumbnailByLongestSideSet(rint(96), [pid1, pid2], {'omero.group': '-1'})
    serverExceptionClass = ome.conditions.SecurityViolation
    message = Group is ------. Cannot link to object: ome.model.core.Pixels:Id_39754

// But without the group -1 context, this works
 tb.getThumbnailByLongestSideSet(rint(96), [pid1, pid2])
// And NOW, this will work again too!
tb.getThumbnailByLongestSideSet(rint(96), [pid1, pid2], {'omero.group': '-1'})

Do you want me to write a Python integration test for this in a separate branch?

will-moore · 2017-03-30T22:58:55Z

@aleksandra-tarkowska
This works now with group -1 (with images from different groups).

tb.getThumbnailByLongestSideSet(rint(96), [pid1, pid2], {'omero.group': '-1'})

So I've re-opened my PR above #5192 and reverted your commit that used group: None.
Everything works fine except after you "Save to All" (see last code sample above).

atarkowska · 2017-03-30T23:00:29Z

@will-moore feel free to open against #5158 so we will reduce number of dependent branches

#5192 can be closed now

joshmoore · 2017-03-31T06:56:45Z

components/server/src/ome/services/ThumbnailBean.java

+                try {
+                    applicationContext.publishMessage(new ContextMessage.Push(this, groupContext));
+                } catch (Throwable t) {
+                    log.error("could not publish context change push", t);


Should the exceptions on push/pop not be thrown?

mtbc · 2017-03-31T07:13:02Z

Do you want me to write a Python integration test for this in a separate branch?

Thank you for the offer! I'll start out by trying to reproduce this as part of the existing Java test class.

snoopycrimecop · 2017-04-03T02:31:22Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#648. See the console output for more details.
Possible conflicts:

PR Get thumbnails (rebased onto develop) #5158 aleksandra-tarkowska 'Get thumbnails (rebased onto develop)'
- components/server/src/ome/services/ThumbnailBean.java

atarkowska · 2017-04-04T20:02:48Z

--rebased-to #5234

will-moore · 2017-04-05T14:13:04Z

Test created at #5235

mtbc · 2017-04-13T13:15:30Z

To check: this PR may break thumbnails in shares.

atarkowska · 2017-04-13T13:20:15Z

is share workflow not going to be removed in 5.3.x ?

mtbc · 2017-04-13T13:21:30Z

I don't know that it is.

jburel · 2017-04-13T19:19:47Z

It cannot be removed in 5.3.x, it has just been deprecated.
We still need to be sure that the existing shares work

pwalczysko · 2017-04-18T09:04:07Z

This PR indeed does seem to break thumbnails in Shares. Checked today on eel merge.

mtbc · 2017-04-18T09:57:20Z

@pwalczysko: The latest commit attempts to fix the shares issue.

pwalczysko · 2017-04-20T08:58:11Z

Checked that the PR is in, it seems to be. https://github.com/snoopycrimecop/openmicroscopy/tree/develop/merge/daily.

There is no change in behaviour with respect to shares, the thumb problem persists.

mtbc · 2017-04-20T09:01:02Z

Which user is sharing with which user?

pwalczysko · 2017-04-20T09:01:26Z

user-4 and user-1, and vice versa.

pwalczysko · 2017-04-20T09:02:09Z

The other problem with plates persists as well. #5158 (comment)

mtbc · 2017-04-20T09:23:25Z

In local testing this works with #5207 only

but when I include #5158 in the merge I lose the thumbnail, so I think this PR's fine. (Similarly, recall that the plate problem was fine a couple of days ago when this PR was in but #5158 was still excluded.)

atarkowska · 2017-04-20T09:33:27Z

That would make sense because web in this branch is not using getThumbnailByLongestSideSet. That is the problem we are trying to fix.

mtbc · 2017-04-20T09:36:08Z

Both this PR's integration test and @will-moore's in #5235 use getThumbnailByLongestSideSet.

atarkowska · 2017-04-20T09:37:55Z

For some reason https://github.com/openmicroscopy/openmicroscopy/pull/5158/files#diff-e4feecea30b2ee097639cbb7c9171fedR4455 is not working, it might be worth to reproduce that method in the first place?

atarkowska · 2017-04-20T09:39:11Z

Both this PR's integration test and @will-moore's in #5235 use getThumbnailByLongestSideSet.

I am afraid in both mentioned PRs web loads thumbnail one by one using https://github.com/aleksandra-tarkowska/openmicroscopy/blob/2a66a0c129dd499aae6a88f398057f024a1f8183/components/tools/OmeroPy/src/omero/gateway/__init__.py#L8002

mtbc · 2017-04-28T07:23:50Z

@pwalczysko: Is this PR all good for you now?

pwalczysko · 2017-04-28T09:00:31Z

Yes, ready to merge.

mtbc added 12 commits March 10, 2017 15:03

thumbnail get is graph critical in all-groups context

12e38a6

no suitable context for creating new rendering settings

fix typo in comment

dec4b51

update copyright

3d8a90b

add integration test for fetching thumbnails in all-groups

b89db01

add extra asserts to integration test

7fa3ad7

add try/finally for service closure

76d85bd

suggested by @jburel

Revert "thumbnail get is graph critical in all-groups context"

a9b51db

This reverts commit 12e38a6. It is to be replaced with an alternative fix.

to test get thumbnail set, don't first fetch thumbs separately

84e7a64

triggers an issue noticed by @aleksandra-tarkowska

correct order of assertEquals arguments

e573c22

execute thumbnail store operations in appropriate group context

16a2b5e

fix for https://trello.com/c/bJqd3SkJ/

inconsequential code tweaks in thumbnail bean

87f7f89

in thumbnail context add some Javadoc and tighten visibility

572ec50

mtbc mentioned this pull request Mar 29, 2017

do not attempt to create rendering settings in all-groups context #5172

Closed

jburel added the develop label Mar 29, 2017

mtbc closed this Mar 30, 2017

mtbc reopened this Mar 30, 2017

atarkowska mentioned this pull request Mar 30, 2017

Get thumbnails (rebased onto develop) #5158

Closed

joshmoore reviewed Mar 31, 2017

View reviewed changes

mtbc closed this Mar 31, 2017

mtbc reopened this Mar 31, 2017

mtbc force-pushed the thumbnail-context branch from 74d020c to 1f81fc2 Compare March 31, 2017 13:39

allow specifying "no group" as null instead of -1

d9e3a13

This was referenced Apr 4, 2017

Get thumbnails (rebased onto metadata53) #5233

Merged

allow creation of thumbnails in all-groups context (rebased onto metadata53) #5234

Merged

will-moore mentioned this pull request Apr 5, 2017

getThumbnails_test #5235

Merged

shares are always extended graph critical

cb80460

will-moore mentioned this pull request Apr 20, 2017

Get thumbnails #5192

Merged

jburel merged commit 535c5b2 into ome:develop May 9, 2017

mtbc deleted the thumbnail-context branch May 9, 2017 13:44

jburel added this to the 5.3.2 milestone May 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow creation of thumbnails in all-groups context #5207

allow creation of thumbnails in all-groups context #5207

mtbc commented Mar 29, 2017 •

edited

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

mtbc commented Mar 30, 2017 •

edited

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

atarkowska commented Mar 30, 2017 •

edited

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

atarkowska commented Mar 30, 2017 •

edited

joshmoore Mar 31, 2017

mtbc commented Mar 31, 2017

snoopycrimecop commented Apr 3, 2017

atarkowska commented Apr 4, 2017

will-moore commented Apr 5, 2017

mtbc commented Apr 13, 2017

atarkowska commented Apr 13, 2017 •

edited

mtbc commented Apr 13, 2017

jburel commented Apr 13, 2017 •

edited

pwalczysko commented Apr 18, 2017

mtbc commented Apr 18, 2017

pwalczysko commented Apr 20, 2017

mtbc commented Apr 20, 2017

pwalczysko commented Apr 20, 2017

pwalczysko commented Apr 20, 2017

mtbc commented Apr 20, 2017

atarkowska commented Apr 20, 2017 •

edited

mtbc commented Apr 20, 2017

atarkowska commented Apr 20, 2017

atarkowska commented Apr 20, 2017

mtbc commented Apr 28, 2017

pwalczysko commented Apr 28, 2017

allow creation of thumbnails in all-groups context #5207

allow creation of thumbnails in all-groups context #5207

Conversation

mtbc commented Mar 29, 2017 • edited

What this PR does

Testing this PR

Related reading

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

mtbc commented Mar 30, 2017 • edited

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

atarkowska commented Mar 30, 2017 • edited

will-moore commented Mar 30, 2017

will-moore commented Mar 30, 2017

atarkowska commented Mar 30, 2017 • edited

joshmoore Mar 31, 2017

Choose a reason for hiding this comment

mtbc commented Mar 31, 2017

snoopycrimecop commented Apr 3, 2017

atarkowska commented Apr 4, 2017

will-moore commented Apr 5, 2017

mtbc commented Apr 13, 2017

atarkowska commented Apr 13, 2017 • edited

mtbc commented Apr 13, 2017

jburel commented Apr 13, 2017 • edited

pwalczysko commented Apr 18, 2017

mtbc commented Apr 18, 2017

pwalczysko commented Apr 20, 2017

mtbc commented Apr 20, 2017

pwalczysko commented Apr 20, 2017

pwalczysko commented Apr 20, 2017

mtbc commented Apr 20, 2017

atarkowska commented Apr 20, 2017 • edited

mtbc commented Apr 20, 2017

atarkowska commented Apr 20, 2017

atarkowska commented Apr 20, 2017

mtbc commented Apr 28, 2017

pwalczysko commented Apr 28, 2017

mtbc commented Mar 29, 2017 •

edited

mtbc commented Mar 30, 2017 •

edited

atarkowska commented Mar 30, 2017 •

edited

atarkowska commented Mar 30, 2017 •

edited

atarkowska commented Apr 13, 2017 •

edited

jburel commented Apr 13, 2017 •

edited

atarkowska commented Apr 20, 2017 •

edited