Don't use group -1 for luts_png #603

will-moore · 2025-01-07T13:25:05Z

Issues that need addressing:

We can't rely on cache being enabled for omero-web
Dynamic generation of all LUTs takes a while and loads a lot of files from the server

Proposed solution:

We use the a new file webgateway/static/json/luts.json as the source for LUTs to build the dynamic /webgateway/luts_png/ (the /webgateway/luts_png/ still corresponds to the dynamic /luts/ JSON that comes from the server. Any LUTs from the server that are not in the static luts.json will be shown as white in /webgateway/luts_png/.
If you use /webgateway/luts/rgb=true then the JSON produced will include the rgb values for each LUT. This uses the static luts.json, but if there are new LUTs coming from the server (NOT in the luts.json) then we load those LUT files from the server.
The workflow for updating the LUTs in the static luts.json is to simply take the JSON output from /webgateway/luts/rgb=true and save it into webgateway/static/json/luts.json.

NB: The static luts.json in this PR doesn't yet have the "new" LUTs recently added. This enables us to test a few things. Then, we can update the luts.json with recent LUTs before merging this PR...

To test:

Go to /webgateway/luts/ - this will be unchanged from before. Compare to /webgateway/luts/?rgb=true which has rgb for every LUT - each is an array of shape (256, 3). The recently added LUTs that are not yet in the static luts.json (e.g. cividis.lut) are being generated on the fly from the server.
Go to /webgateway/luts_png/. This is now being generated from the rgb values in the static luts.json, instead of loading LUT files from the server. This is very fast, so caching functionality is no-longer needed (has been removed). You will see white gaps for LUTs that are not yet cached in the static luts.json. ]
Check README for instructions on updating the static luts.json

will-moore · 2025-01-07T13:26:07Z

cc @Tom-TBT

Tom-TBT · 2025-01-07T13:29:01Z

Thank you for the fix Will, sorry for that.

will-moore · 2025-01-07T13:47:24Z

@Tom-TBT This is some strange behaviour we are seeing ONLY on our production server (which has a very long history), but not on any other server we've tested on. So, nothing to apologise for!

jburel · 2025-01-07T15:21:45Z

after investigation it seems that the wrong permissions are set for the user group on the production server. This PR is not needed

will-moore · 2025-01-15T14:20:01Z

We are seeing significant performance improvements with this change if the user's default group has a large number of users, so it seems to be worth including.

Tom-TBT · 2025-01-15T15:09:56Z

Interesting. I'm curious, can you explain why the performance is affected here?
Without setting group to -1, does getObject search first for all users in the group before searching outside of the group?

will-moore · 2025-01-16T12:38:52Z

@Tom-TBT The issue of slow cross-group queries when a user is in a big group are very low-level OMERO internals. I'm not sure if it's documented anywhere...

Another issue we're finding is that we can't assume that omero-web installations will have caching enabled.

The default behaviour is for caching to be disabled. Default is a dummy cache:

omero-web/omeroweb/settings.py

Line 456 in 96d1138

('{"default": {"BACKEND":' ' "django.core.cache.backends.dummy.DummyCache"}}'),

and configuring e.g. redis is described as optional:

https://omero.readthedocs.io/en/stable/sysadmins/unix/install-web/walkthrough/omeroweb-install-rockylinux9-ice3.6.html#running-omero-web

So, what is best to do? Options:

Update omeroweb/settings.py to use some valid cache config by default. E.g. https://docs.djangoproject.com/en/5.1/topics/cache/#filesystem-caching where we'd need to figure out a suitable LOCATION and there's warnings about locations within MEDIA_ROOT , STATIC_ROOT, or STATICFILES_FINDERS.
or django.core.cache.backends.locmem.LocMemCache - not ideal for production but could be fine as a default for occasional use such as luts?
Update our docs (and code) to try and get all users to enable caching? Seems like a big task just for this usage of caching, but maybe we want to take more advantage of caching if we know it's available?
As an alternative to caching luts_png in Django, we could resort to saving it within OMERO. E.g. a shared OriginalFile with cache_key as the name? Not as nice a caching in Django, but certainly better than no caching at-all.
Any other options? Could ask admins to run some luts_png generation on the server after updating LUTS, but not very nice!

will-moore · 2025-01-16T16:30:47Z

Tested setting the group to the user group with this addition to the script above:

user_group_id = conn.getAdminService().getSecurityRoles().userGroupId
conn.SERVICE_OPTS.setOmeroGroup(user_group_id)

And logged-in to nightshade as test-user who's default group has many users.

However, this had no difference in performance compared with leaving the group as the user's default group. Only setting group: -1 causes a slow-down.

for more information, see https://pre-commit.ci

will-moore · 2025-01-20T16:07:43Z

cc @jburel @Tom-TBT I've updated the description to correspond to the proposed solution now in this PR, with options TBD...

pwalczysko · 2025-01-24T12:56:51Z

NB: for testing this PR, use /webgateway/luts_png/?cached=false to disable the cache.

Could you please expand on how to test ?

I went on merge-ci as user-3 to url

https://merge-ci.openmicroscopy.org/web/webgateway/luts_png/

and it loaded.

When I went to

https://merge-ci.openmicroscopy.org/web/webgateway/luts_png/webgateway/luts_png/?cached=false

then it loaded too.

There was no perceptible speed differentce between the two cases ^^^

will-moore · 2025-01-24T15:15:42Z

Discussed at web meeting today...
It would be nice to replace usage of LUTS_IN_PNG list and static luts_10.png with a JSON object that combines both the LUT names alongside the 256 rgb values of the LUT.

for more information, see https://pre-commit.ci

README.rst

jburel · 2025-01-30T10:07:25Z

README.rst

+cached in https://github.com/ome/omero-web/blob/master/omeroweb/webgateway/static/webgateway/json/luts.json.
+The LUTs in the `/luts_png/` will always correspond to the LUTs on the server as available in JSON
+from `/webgateway/luts/`.
+If new LUTs are added to the server and are not found in the `luts.json` then the `/luts_png/` will


again confusing.
luts.json is a visual representation i.e. name and associated png

No, luts.json is a json file (see this PR).

I know, I should have said "holds a visual representation but If new LUTs are added to the server and are not found in the `luts.json is not clear

I rewrote the README - hopefully more clear now?

sbesson

Immediate thoughts while reading this proposal is that it brings us back to the coupling issues originally raised in #568. When new LUTs get added to the server, the following must happen
1- a release of OMERO.web with an updated luts.json
2- possibly a release of all web apps of the ecosystem to depend on the newest version of OMERO.web

Has there been any consideration about hybrid solution where the cached JSON would be retrieved and only the missing LUTs would be dynamically fetched from the server when retrieving the PNG representation?

sbesson · 2025-02-05T11:05:47Z

README.rst

+
+The OMERO server ships with a set of look-up tables (LUTs) for rendering images. Users can also
+add their own LUTs to the server. The LUTs available on the server can be retrieved from the
+`/webgateway/luts/` endpoint as JSON data.


Although it certainly does not hurt to have this documentation here, it feels a bit at odds with the rest of the README.

Should https://omero.readthedocs.io/en/stable/developers/Web/WebGateway.html be extended to cover the LUT endpoints of the OMERO.web gateway?

jburel · 2025-02-05T11:30:46Z

The Upgrade of LUTS does not happen very often (first time)
The dynamic change was explored by @will-moore but due to the rarity of the upgrade and do not see the synch release a major burden
The situation has improved since there is now no hard-coded list of items in web apps.
the json file also ensures that the visual representation matches the correct lut

sbesson

I appreciate the LUT upgrade is not frequent and there is so much time we want to invest into the dynamic aspect. Especially if the next OMERO.web release will also include the upgraded JSON with the LUTs.

I was primaril asking as the dynamic loading is implemented in the /webgateway/luts&rgb=true and this endpoint could possibly be used rather than loading the cache in /webgateway/luts_png.

Performance-wise, comparing the timings of https://merge-ci.openmicroscopy.org/web/webgateway/luts/?rgb=true to https://merge-ci.openmicroscopy.org/web/static/webgateway/json/luts.json, the former call responds with a couple of 100 additional milliseconds (~574ms vs. 257ms). So there is an open question of which metrics is acceptable.

At the API level, the new rgb=true parameters in the /webgateway/luts/ endpoint are backwards compatible. However, as described in the PR, the behavior of /webgateway/luts_png/ is completely modified. At minimum, the docstring should be updated to reflect the new expectation. This also raises the question of whether these changes are significant enough that they should be considered as backwards incompatible .
Testing wise, the previous implementation suggests updates to the LUT name/path would be reflected in these endpoints. Is this also true in the new implementation and should this be tested?

sbesson · 2025-02-05T12:33:26Z

omeroweb/webgateway/views.py

-        )
+        # rgb = load_lut_to_rgb(conn, lut.id.val)
+        lut_data = {
+            "id": lut.id.val,


The LUT ID might vary from server to server. This is not a problem per se especially as you are using path+name for normalizing but this means there is a mismatch between the cached JSON file and this endpoint - which can be seen for instance by comparing https://merge-ci.openmicroscopy.org/web/webgateway/luts/ with https://merge-ci.openmicroscopy.org/web/static/webgateway/json/luts.json

Should id not omitted in the cache?

Yes ids will change, but are harmless. If we want to allow the easy copying of /webgateway/luts/?rgb=true into the static luts.json but ALSO exclude ids, then maybe we need another parameter ?ids=false. Easy to do if worth it?

jburel · 2025-02-05T13:06:40Z

README.rst

+LUTs caching
+------------
+
+The OMERO server ships with a set of look-up tables (LUTs) for rendering images. Users can also


Admin can add, not users since it is treated as a script

Fixed in 961d307

will-moore · 2025-02-05T13:30:24Z

Thanks for the in-depth reviews...

Has there been any consideration about hybrid solution where the cached JSON would be retrieved and only the missing LUTs would be dynamically fetched from the server when retrieving the PNG representation?

I actually think this would make a lot of sense. The performance issues we saw previously were because we were fetching all 47 LUTs from the server, whereas if you're only fetching 1 or 2 then this won't be a problem. This would mean the behaviour of /webgateway/luts_png/ would be unchanged (always renders all LUTs that are on the server).

The 574ms timing for https://merge-ci.openmicroscopy.org/web/webgateway/luts/?rgb=true above includes loading ~8 LUTs from the server (which are not yet in the static luts.json).

We could consider re-enabling the caching of the /luts_png/, for those rare times when LUTs are added (but it wouldn't be essential for users to enable caching).

Updates to LUTs name/path will be reflected in the /webgateway/luts json. Testing would be nice but is not so easy and if there is an issue then it's likely due to the OMERO api since all we're going here is converting that output to JSON.

jburel · 2025-02-05T21:40:51Z

Updates to LUTs name/path will be reflected in the /webgateway/luts json. Testing would be nice but is not so easy and if there is an issue then it's likely due to the OMERO api since all we're going here is converting that output to JSON.

A test focusing on the OMERO API call will help in that case

will-moore · 2025-02-06T10:51:00Z

I think adding tests for the OMERO API (if they don't already exist) is probably outside the scope of this PR.
I'm not exactly sure how a test would check that the LUTs listed by the script service match what's on the server?

To focus on what needs to be done on this PR:

should I update the webgateway/luts_png/ to load the non-cached LUTs from the server (as we do for /luts/?rgb=true JSON)?
should I add back Django caching for webgateway/luts_png/ as before this PR (for the times when it could be useful)?

EDIT: discussed 7th Feb web meeting - "Yes" to both those questions...

will-moore · 2025-02-09T19:58:32Z

To test the performance of loading 8 LUTs from the server, compare response times for /webgateway/luts_png/?cached=false (no LUTs loaded from server) with /webgateway/luts_png/?cached=false&new=true.

NB: we always use ?cached=false so we don't get cached response. Without that, it should be even faster.

will-moore · 2025-02-12T14:12:31Z

Tested on merge-ci by alternatively running these 2 commands in the webclient Devtools Console, then checking the response times in the Network tab:

fetch("https://merge-ci.openmicroscopy.org/web/webgateway/luts_png/?cached=false")
fetch("https://merge-ci.openmicroscopy.org/web/webgateway/luts_png/?cached=false&new=true")

The &new=true call, when we are loading 8 LUTs from the server takes about 400-500ms server response time (not including the SSL time, connection time etc) compared to about 200ms for the other call, when we're not loading LUTs from the server.

To err on the side of safety, I suggest:

By default we DON'T load LUTs from the server.
If a LUT is added to the server, it won't initially show up in the webgateway/luts_png/ (white placeholder instead).
If the server has cache enabled, then a single visit to webgateway/luts_png/?cached=false&new=true (E.g by an Admin) will be sufficient to load the LUTs once and populate the Django cache with the complete png, so that all other users will benefit.

This is actually the current behaviour. So if this sounds good, I can update the README to explain this workflow then we should be good to go?

will-moore · 2025-02-12T14:16:37Z

I'll add the "new" LUTs into the static file now...

sbesson

This PR tries to fix the LUT retrieval in two separate deployments configuration:

if no OMERO.web cache backend is configured (via omero.web.caches), the implementation will always load the static JSON file and return a preview which might include white gaps if additional LUTs are present server-side unless new=true is passed. This is consistent with my expectation i.e. the implementation should avoid reloading additional LUTs for every single request to the endpoint
if an OMERO.web cache backend is configured e.g. Redis
- if a cache entry with a key hashed from the list of LUTs exists and cached=False is not set, it is returned as a response
- otherwise, as above the static JSON file is read and used to generate a preview image. White gaps might be present if there are more LUTs server-side than in the static JSON and new=true is not passed
- the preview image is then stored in the cache

My primary issue is associated with the second scenario and the impact of the double cache and the issues associated with troubleshooting and invalidating such a cache. One example of complex scenario:

LUTs are added server-side e.g. via a new OMERO.server release
OMERO.web is either not yet released or not deployed with an matching LUTs JSON file
the first call to the LUT preview endpoint finds no cache associated with the new hash. The implementation regenerates a preview image including white gaps and stored in the cache
every subsequent call to the endpoint will read the cached preview image with white gaps even if OMERO.web is upgraded with a new JSON including the new LUTs
the only way to fix this situation would be to invalidate the cache

Is there a way to detect such a scenario and force the LUT loading as a one-off?

In general, the docstrings should be updated to describe the new logic has changed. The README addition is useful but as this is documenting an occasional workflow including possibly server-side change, I think it would be more relevant under https://omero.readthedocs.io/en/stable/.

sbesson · 2025-02-18T09:20:57Z

omeroweb/webgateway/views.py

+        if pathname in luts_by_pathname:
+            lut_rgb = luts_by_pathname[pathname].get("rgb")
+            new_img[(i * 10) : ((i + 1) * 10), :, :3] = lut_rgb
+        elif request.GET.get("new") == "true":


I am unclear on the distinction of the cached and new query parameters. In particular, I would expect that calling cached=false would also new=true. Is there a scenario where we would like to have different combinations?

will-moore · 2025-02-18T10:54:16Z

@sbesson If the luts_png wasn't saved to Django cache unless ?new=true then I think that would address the issues with your 2nd scenario? There's really no point in caching the png that has whitespaces (and is only generated from the static luts.json).

I also think you're right about cached=false and new=true always going together. So I'll drop the new=true and just use cached=false.

So, the luts_png call has 2 behaviours:

cached=false - Ignore Django cache, load LUTs from the server if not in static luts.json and save the png in the Django cache
otherwise: Use the Django cache if it exists, and if not then generate the png from the static luts.json.

will-moore · 2025-02-19T15:26:47Z

@sbesson That commit 3bfb3fc should address your points.

Don't use group -1 for luts_png

de444ef

jburel closed this Jan 7, 2025

will-moore reopened this Jan 15, 2025

knabar added this to the 5.29.0 milestone Jan 17, 2025

will-moore added 4 commits January 17, 2025 14:22

Don't use group:-1 for listLuts_json

afc2c7e

Add support for luts_png/?cached=false

f6f4258

Use static/webgateway/img/luts_10.png for /luts_png/

03e6c6a

Add option to generate dynamic luts if not in static png

eb71bb1

will-moore force-pushed the luts_getOriginalFile_group-1 branch from 576d62d to eb71bb1 Compare January 20, 2025 15:46

[pre-commit.ci] auto fixes from pre-commit.com hooks

f1507c1

for more information, see https://pre-commit.ci

Remove debug lut_crop.show()

e30c293

will-moore requested review from pwalczysko and jburel and removed request for pwalczysko January 22, 2025 13:34

will-moore and others added 5 commits January 28, 2025 12:45

Add load_lut_to_rgb() to utils

3490f80

Use static luts.json to cache luts

121fff4

Add LUTs caching notes to README

c0b2ee0

[pre-commit.ci] auto fixes from pre-commit.com hooks

a36306d

for more information, see https://pre-commit.ci

flake8 fix

7a4f809

jburel reviewed Jan 30, 2025

View reviewed changes

README.rst Outdated Show resolved Hide resolved

jburel reviewed Jan 30, 2025

View reviewed changes

will-moore added 2 commits January 30, 2025 15:15

Rewrote the LUTs caching section of README to improve clarity

4d819ce

Format static luts.json to improve future diffs

eada123

sbesson reviewed Feb 5, 2025

View reviewed changes

jburel reviewed Feb 5, 2025

View reviewed changes

Fix Users -> Admins in README luts

961d307

will-moore added 2 commits February 8, 2025 22:03

luts_png/?new=true loads luts from server if needed

297e69b

Use cached png except with /luts_png/?cached=false

f28328e

will-moore added 2 commits February 12, 2025 14:23

Add 8 'new' LUTs into static luts.json

3175ede

Update README with new LUTs cache workflow

d721376

sbesson reviewed Feb 18, 2025

View reviewed changes

Remove ?new=true and only cache if not use_cached

3bfb3fc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use group -1 for luts_png #603

Don't use group -1 for luts_png #603

will-moore commented Jan 7, 2025 •

edited

Loading

will-moore commented Jan 7, 2025

Tom-TBT commented Jan 7, 2025

will-moore commented Jan 7, 2025

jburel commented Jan 7, 2025

will-moore commented Jan 15, 2025

Tom-TBT commented Jan 15, 2025

will-moore commented Jan 16, 2025

will-moore commented Jan 16, 2025

will-moore commented Jan 20, 2025

pwalczysko commented Jan 24, 2025 •

edited

Loading

will-moore commented Jan 24, 2025

jburel Jan 30, 2025

will-moore Jan 30, 2025

jburel Jan 30, 2025

will-moore Jan 30, 2025

sbesson left a comment •

edited

Loading

sbesson Feb 5, 2025

jburel commented Feb 5, 2025 •

edited

Loading

sbesson left a comment

sbesson Feb 5, 2025

will-moore Feb 5, 2025

jburel Feb 5, 2025

will-moore Feb 5, 2025

will-moore commented Feb 5, 2025

jburel commented Feb 5, 2025

will-moore commented Feb 6, 2025 •

edited

Loading

will-moore commented Feb 9, 2025

will-moore commented Feb 12, 2025 •

edited

Loading

will-moore commented Feb 12, 2025

sbesson left a comment •

edited

Loading

sbesson Feb 18, 2025

will-moore commented Feb 18, 2025

will-moore commented Feb 19, 2025

Don't use group -1 for luts_png #603

Are you sure you want to change the base?

Don't use group -1 for luts_png #603

Conversation

will-moore commented Jan 7, 2025 • edited Loading

will-moore commented Jan 7, 2025

Tom-TBT commented Jan 7, 2025

will-moore commented Jan 7, 2025

jburel commented Jan 7, 2025

will-moore commented Jan 15, 2025

Tom-TBT commented Jan 15, 2025

will-moore commented Jan 16, 2025

will-moore commented Jan 16, 2025

will-moore commented Jan 20, 2025

pwalczysko commented Jan 24, 2025 • edited Loading

will-moore commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jburel commented Feb 5, 2025 • edited Loading

sbesson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

will-moore commented Feb 5, 2025

jburel commented Feb 5, 2025

will-moore commented Feb 6, 2025 • edited Loading

will-moore commented Feb 9, 2025

will-moore commented Feb 12, 2025 • edited Loading

will-moore commented Feb 12, 2025

sbesson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

will-moore commented Feb 18, 2025

will-moore commented Feb 19, 2025

will-moore commented Jan 7, 2025 •

edited

Loading

pwalczysko commented Jan 24, 2025 •

edited

Loading

sbesson left a comment •

edited

Loading

jburel commented Feb 5, 2025 •

edited

Loading

will-moore commented Feb 6, 2025 •

edited

Loading

will-moore commented Feb 12, 2025 •

edited

Loading

sbesson left a comment •

edited

Loading