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

imgData JSON with no stateful services #536

Closed

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Feb 23, 2024

It has been noted in some cases that the Preview panel and the right panel of iviewer can take a long time to load, since we currently initialise various stateful services (which requires initialising them with the original image files). This means that the viewers are slower to load than if we just get the data from the DB, particularly if access to original data is slow.

Stateful services usage is replaced as follows:

  • Instead of using rendering engine for settings, we load rendering def
  • Instead of the rawPixelsStore for image.getPixelRange(), we lookup values based on pixel type
  • Instead of using rendering engine for requiresPixelsPyramid() we use image.requiresPixelsPyramid() which simply uses the size cut-off.
  • IF the image is BIG, then we load the rendering engine to get the actual pyramid resolutions

To test:

  • Image viewer should work as before but load faster
  • The only other change in behaviour is for small images that have a pyramid, such as small NGFF images. These will appear as small images in the Preview panel and will load as whole planes instead of tiles in iviewer.
  • Instead of using re.lookupRenderingDef(pid, ctx) for the logic of "use my rdef or use the owners", this is now encoded in webgateway.util.get_rendering_def()
  • If no rendering def exists, we use rendering engine to create one as before.

Also see performance testing with NGFF data on idr-testing at #537 (comment)
Summarised as follows:

  • Don't expect this PR to noticeably improve performance for most data in OMERO
  • For data where we do notice a significant improvement (e.g. idr0090 NGFF data) even if we always use RenderingEngine to test/load the pyramid levels (ie. NO api change), this PR still has a performance improvement.

@will-moore will-moore force-pushed the imgData_json_without_stateful_services branch from d17518c to 944dd03 Compare February 24, 2024 06:03
@will-moore will-moore force-pushed the imgData_json_without_stateful_services branch from a074c29 to cc3f741 Compare February 24, 2024 06:52
@will-moore will-moore force-pushed the imgData_json_without_stateful_services branch from 8bfba4c to a3cb187 Compare February 25, 2024 06:20
@will-moore
Copy link
Member Author

Deploy on idr-testing...

for server in omeroreadwrite omeroreadonly-1 omeroreadonly-2 omeroreadonly-3 omeroreadonly-4; do ssh $server "sudo /opt/omero/web/venv3/bin/pip uninstall -y omero-web && sudo /opt/omero/web/venv3/bin/pip install git+https://github.com/ome/omero-web.git@refs/pull/536/head && sudo service omero-web restart"; done

This failed since it's still python 3.6 and this branch (based on latest omero-web) requires Django 4.2!
Reverted...

for server in omeroreadwrite omeroreadonly-1 omeroreadonly-2 omeroreadonly-3 omeroreadonly-4; do ssh $server "sudo /opt/omero/web/venv3/bin/pip install omero-web && sudo service omero-web restart"; done

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-python-superbuild-push#470. See the console output for more details.
Possible conflicts:

--conflicts

@will-moore
Copy link
Member Author

will-moore commented Feb 28, 2024

EDIT - svs issue identified here moved to IDR/idr-metadata#691

@knabar knabar added this to the 5.25.0 milestone Mar 1, 2024
@snoopycrimecop
Copy link
Member

snoopycrimecop commented Mar 6, 2024

Conflicting PR. Removed from build OMERO-python-superbuild-push#477. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build OMERO-python-superbuild-push#478. See the console output for more details.

@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@ome ome deleted a comment from snoopycrimecop Mar 6, 2024
@knabar knabar removed this from the 5.25.0 milestone Mar 12, 2024
@will-moore will-moore requested a review from knabar March 13, 2024 12:24
@knabar
Copy link
Member

knabar commented Mar 20, 2024

There are still some concerns about this potentially introducing breaking API changes.

Are issues like #536 (comment) still happening?

Performance improvements seem to be limited to particular image types (#537 (comment)).

The way image pyramids/tiles are detected changes the behavior of the API endpoint.

  • Could this be introduced as a new API endpoint instead, for individual clients to use once it's verified that there are no issues with that particular client?
  • Or alternatively, add a query argument like ?skipRenderingEngine without which the previous behavior remains exactly?

Both those options would guarantee that no clients break in an unexpected fashion with any images, and clients that might benefit could be updated to use the new behavior with a single line change to the new endpoint or by adding the query argument.

@will-moore
Copy link
Member Author

Sorry - please ignore #536 (comment) - I just happened to find that bug when testing this PR so I reported it here, but it's not related to this PR.

I'll revert the API changes as suggested (always use renderingEngine to check for pyramids), then we can discuss if it's worth adding a skipRenderingEngine option...

@will-moore will-moore force-pushed the imgData_json_without_stateful_services branch from 82149cf to ff4f516 Compare March 21, 2024 11:08
@will-moore
Copy link
Member Author

@knabar I replaced much of the code in the same order as before, so the PR changes are reduced.
One change in the last commit is to not return early if the rendering engine fails to load, since we can maybe still add other useful info, particularly if we can load rendering defs.

@knabar
Copy link
Member

knabar commented Mar 21, 2024

I set up a temporary branch that exposes both the original and the new imgData endpoint at https://github.com/knabar/omero-web/tree/temp-try-imgdata-json

Using the following commands on bash to compare the outputs, I tried a number of different images:

ID=347168
COOKIE='Cookie: csrftoken=...; sessionid=...'
diff <(curl -s http://localhost:4080/webgateway/imgDataOriginal/${ID}/ -H ${COOKIE} | jq --sort-keys .) \
  <(curl -s http://localhost:4080/webgateway/imgData/${ID}/ -H ${COOKIE} | jq --sort-keys .)

So far the main difference is with the channels min/max being returned as integers instead of floats, but more unusual images still need to be tested.

14,15c14,15
<         "max": 65535.0,
<         "min": 0.0,
---
>         "max": 65535,
>         "min": 0,
30,31c30,31
<         "max": 65535.0,
<         "min": 0.0,
---
>         "max": 65535,
>         "min": 0,
46,47c46,47
<         "max": 65535.0,
<         "min": 0.0,
---
>         "max": 65535,
>         "min": 0,

cc: @sbesson

@will-moore
Copy link
Member Author

Re: #536 (comment) float vv ints:
Testing with floating-point images (with #548 deployed on idr-testing) I see that the window values are also floats:
https://idr-testing.openmicroscopy.org/webclient/imgData/1884808/

"window": {
  "min": -2147483648,
  "max": 7880.49462890625,
  "start": 2509.07969,
  "end": 5488.61182
}

@will-moore
Copy link
Member Author

For some images where window.min == 0 (darkest pixel value is 0) we are seeing incorrect values in the JSON, particularly noticeable with big negative numbers with float images, e.g. idr0021 image:

"window": {
    "min": -2147483648,
    "max": 1678.546630859375,
    "start": 367.756,
    "end": 779.353
}

The min here is actually the minimum pixel_range value.
This is fixed by 412fa66 above

@will-moore will-moore mentioned this pull request Apr 12, 2024
@will-moore
Copy link
Member Author

As discussed in web meeting today, let's test on idr-testing without deploying this PR (via branch at #548) and see if it's really needed for idr0090 or if other improvements are sufficient to provide acceptable performance and stability.

Deployed vanilla omero-web with

for server in omeroreadwrite omeroreadonly-1 omeroreadonly-2 omeroreadonly-3 omeroreadonly-4; do ssh $server "sudo /opt/omero/web/venv3/bin/pip uninstall -y omero-web && sudo /opt/omero/web/venv3/bin/pip install omero-web && sudo service omero-web restart"; done
...
Successfully installed omero-web-5.22.1

@will-moore
Copy link
Member Author

Since it appears we don't now need these changes for IDR, closing....

@will-moore will-moore closed this Apr 30, 2024
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.

3 participants