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

Max projection bytes #560

Merged
merged 15 commits into from
Sep 2, 2024
Merged

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented May 2, 2024

Fixes #440.

This protects users from requesting Z-projections exceeding the MAX_PROJECTION_BYTES limit of 1024 * 1024 * 256 bytes or a value configured on the server omero.pixeldata.max_projection_bytes.
We always take into account the number of active channels rather than total channel count.

When a user enables Z-projection on an image, we test whether the full Z-range projection (with current channels enabled) exceeds the MAX_PROJECTION_BYTES. If so, the maximum number of Z-planes that can be projected is shown in red (tooltip includes the MAX_PROJECTION_BYTES limit). This limit will change when channels are turned on/off.

If a user exceeds the Z-projection limit using the Z-range sliders, we show a warning message instead of the Z-projected image.

Screenshot 2024-05-02 at 16 55 31

To test: (I have already created https://merge-ci.openmicroscopy.org/web/figure/file/3901 as follows):

  • BEFORE this PR is merged, find/create one or more images that are bigger than 1024 * 1024 * 256 bytes. Might be easiest to create fake images. E.g. I tested with testProjection&sizeX=1952&sizeY=1952&sizeZ=73&sizeC=4&pixelType=int16.fake based on https://forum.image.sc/t/z-projections-disabled/95042 and testProjection&sizeX=1024&sizeY=1024&sizeZ=50&sizeC=4&pixelType=int8.fake which falls under the limit when a single channel is enabled.

  • Save a Figure that includes these images with Z-projection exceeding the limit.

  • WITH this PR: Open your existing OMERO.figure and check in the Info panel that correct pixelsType is shown (this data isn't included in existing figures but is loaded on the fly when you open an older figure).

  • Add these to a figure - they should also show the correct pixelsType in Info panel.

  • Enable Z-projection for large and small images. Only those that can exceed the max projected bytes (with the current channels enabled) will show the red Max N with a tooltip to show the meaning.

  • Drag the Z-range sliders - when you choose a range that exceeds the limit in red, you should see the placeholder warning shown instead of requesting the server to project the image:

@sbesson
Copy link
Member

sbesson commented May 3, 2024

From a preliminary round of testing, this functionality seems to work as intended and prevents projections in the app when the selected Z-range exceeds the threshold configured server-side.
One thing I noticed though is that the projection still happens when running the figure export script. I assume the script must also be updated accordingly but this raises the question of what the behavior should be when a figure has been saved with a Z-range exceeding the max projection limit threshold.

…-render image

Since it may be that Z-projection now falls within the MAX_PROJECTION_BYTES limit
@will-moore
Copy link
Member Author

@sbesson in 447144c I've updated the figure export script to respect the MAX_PROJECTION_BYTES limit.
If that limit is exceeded then we render a black image (similar to screenshots above but without the warnings).

I think this is reasonable behaviour both for pre-existing figures saved with the Z-projection that exceeds the limit, as well as for newly created figures. In both cases the user will see that the limit has been exceeded in the browser as above before trying to run the export script.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@will-moore testing this functionality with a set of 3D images, I think part of the implementation needs to be modified. In particular

If so, the maximum number of Z-planes that can be projected is shown in red (tooltip includes the MAX_PROJECTION_BYTES limit). This limit will change when channels are turned on/off.

makes sense conceptually but does not match the current semantics of the server projection service. As in ome/omero-web#115, if the total size of sizeX * sizeY * sizeZ * number_of_active_channels * bytesPerPixels exceed the maximum projected bytes limit, then projection needs to be disabled in OMERO.figure unconditionally and independently of the selecting projection range.

@will-moore
Copy link
Member Author

@sbesson Why should the sizeZ influence the ability to perform small Z-range projections?
For example, if I can do Z-projection with a Z-range of 5 on an image of sizeZ: 10, why wouldn't I be allowed to do the same Z-range projection (same number of bytes) on an image with sizeZ: 100?

As I understood, the unconditional disabling of Z-projection in iviewer and webgateway viewer was due to this being simpler to implement (e.g. we don't base this on active channels but on sizeC since it's easier). I'm not sure if this is what you mean by "the current semantics of the server projection service"?

The default behaviour in OMERO.figure when you turn on Z-projection is to start with a small Z-range since this is often what users want and it is less work for the server. It would be nice to preserve this functionality when it is operating below the MAX_PROJECTED_BYTES.

@will-moore
Copy link
Member Author

When combined with #563 this PR causes an error because re_path is no-longer imported.

09:32:53   File "/home/omero/workspace/OMERO-web/.venv3/lib64/python3.9/site-packages/omero_figure/urls.py", line 36, in <module>
09:32:53     re_path(r'^max_projection_range_exceeded/'
09:32:53 NameError: name 're_path' is not defined

We will try to get that PR merged first - excluding this one for now... (editing description)

@sbesson
Copy link
Member

sbesson commented Jul 5, 2024

@will-moore what's the status of this PR? Is it ready for another round of testing?

@sbesson
Copy link
Member

sbesson commented Jul 11, 2024

With the last commits, using the example figure linked from the description

Screenshot 2024-07-11 at 10 08 35

For images whose Z-stack exceeds the projection limit, the tooltip of the projection icon correctly indicates Z-projection exceeds MAX_PROJECTION_BYTES: <value> but cliking on it gives the broken image icon. In addition, reloading figures displays the spinning icon for images meeting this condition. I assume this is not the expected behavior and these should be replaced by the placeholder black plane ?

@will-moore
Copy link
Member Author

@sbesson Thanks, yes I realised that I used urls.path for a url that contained regex. Fixed now.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with sample synthetic image above and below the default projection limit including test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeC=5.fake and test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeC=6.fake.

The app behaves as expected and selectively disables the Z-projection. The export script still uses the Z-projection range and should be updated to match the rest of the application.

One usability discrepancy is that the maximum projection limit is computed from the number of active channels. This matches the request sent to the server but is at odds with the decisions made for both the legacy viewer and OMERO.iviewer - see discussions at ome/omero-iviewer#305, ome/omero-web#115 and ome/omero-web#130.

re_path(r'^max_projection_range_exceeded/'
r'(?P<iid>[0-9]+)/(?:(?P<z>[0-9]+)/)?(?:(?P<t>[0-9]+)/)?$',
views.max_projection_range_exceeded,
name='max_projection_range_exceeded'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since urls.py has been updated to use the new path style in 0eba85e, should this new route be converted to use the new converter specification?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that the url path format allows for optional parameters as we have in the regex here. https://docs.djangoproject.com/en/5.1/ref/urls/#django.urls.path. Tried searching and this suggests that it's not supported. Since we know that the re_path works, let's stick with that for now.

omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py Outdated Show resolved Hide resolved
This also updates the message on the placeholder image to 'Z-projection disabled' rather than exceeded.
This also disables the Z-projection button if Z-projection is disabled and is currently off (we don't want
to prevent users from turning Z-projection off by disabling the button)
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performed another round of review with a set of test images

  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=250&sizeC=1.fake
  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=300&sizeC=1.fake
  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeC=5.fake
  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeC=6.fake
  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeT=5.fake
  • test&sizeX=1024&sizeY=1024&pixelType=uint8&sizeZ=50&sizeT=6.fake

With the default OMERO maximum projection limit, the expectation is that projection should only be disabled for the images with 300 Z-sections and with 50 Z-sections and 6 channels. This matches the behavior of the application

Screenshot 2024-08-22 at 10 42 39

Running the export script and watching for the server logs, I confirm that ome.services.RenderingBean.projectStack is only called 6 times in total as expected

Increasing the maximum projection limit server-side to 512MB via the omero.pixeldata.max_projection_bytes, I am able to toggle the projection for all images. Running the figure export scripts on panels with projection enabled creates 10 calls to the ome.services.RenderingBean.projectStack API as expected.

Unsetting omero.pixeldata.max_projection_bytes and restarting the server, the reloaded figure now displays the Max Z projection disabled in the application for the two images above the limit

Screenshot 2024-08-22 at 10 38 02

Running the export script calls the ome.services.RenderingBean.projectStack API 6 times in total. The exported figure has black planes as placeholders.

It is possible to come back to the initial scenario by toggling off the projection to only display/export the selected Z plane. In that situation, the projection button then gets deactivated as per the maximum projection limit check as expected.

From my side, all scenarios are working as expected including changes in the server-side configuration and the implemented changes should protect the server (and eventually the users) from making unexpected large projection calls.

The only scenario I have not explicitly tested is the upgrade from a previous OMERO.figure version to the new one, both to confirm that the new metadata is added correctly and that a panel exceeding the maximum projection limit would be handled as above. If this is worth it, I can schedule another round of testing with the sample files above.

@will-moore
Copy link
Member Author

will-moore commented Aug 29, 2024

Testing the upgrade by temp excluding this from merge-ci...

Created pre-update figure. Removed exclude flag...

@will-moore
Copy link
Member Author

Using the same images as @sbesson above, I created a figure on merge-ci without this PR and turned-ON Z-projection for all images, then saved the figure.
After updating the build to include this PR, I opened the saved figure. Initially 4 of them were "projection disabled" since we assume dtype is float while the dtype was loaded. Once dtypes are loaded (couple of seconds), only 2 of the images were "projection disabled":

Screenshot 2024-08-29 at 21 45 45

Once this figure was saved and re-loaded, the 2 images were immediately "projection disabled".

LGTM 👍

@snoopycrimecop snoopycrimecop mentioned this pull request Aug 31, 2024
@will-moore will-moore merged commit b540416 into ome:master Sep 2, 2024
1 check passed
@will-moore
Copy link
Member Author

This is now released in OMERO.figure 7.1.0

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.

Max projection limits
2 participants