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

Allow the PixelsService to be runtime configured #5

Merged
merged 5 commits into from
May 3, 2024

Conversation

chris-allan
Copy link
Member

Configurability allows us to turn on and off the ZarrPixelsService as well as swap in another PixelsService with different semantics if required.

If left with the defaults the ZarrPixelsService will be used. This can be checked on startup by looking for a log line such as:

...
2024-04-30 09:14:41,324 INFO  [         c.g.omero.ms.core.PixelsService] (      main) Zarr metadata and array cache size: 500
...

If omero.pixeldata.pixels_service is set to /OMERO/Pixels the default PixelsService will be used and there will be an absence of the above line on startup.

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 in the context of an OMERO.server including omero-zarr-pixel-buffer built from this PR.

Setting omero.pixeldata.pixels_service to /OMERO/Pixels is sufficient to use the default ome.io.nio.PixelsService bean. Using an OME-XML image populated with an externalInfo pointing to an OME-Zarr, the workflow of setting new rendering settings in the Web client leads to the generation of black thumbnails as expected. After reverting the configuration change and restarting the server, the thumbnails are successfully generated.

Proposing to merge this so that we can start consuming a version of the new omero-zarr-pixel-buffer with the extensibility mechanism in various components (server, micro-services)

As immediate next steps, proposing to look into a few issues noticed during the testing:

  • the new configuration property cannot be used directly in the the image-region micro-service which extends the com.glencoesofware.omero.ms.core.PixelsService. Additional work is required in order to allow the micro-service PixelsService to be configured at runtime
  • when starting the server with omero.pixeldata.pixels_service=/OMERO/Pixels, two PixelsService are initialised at startup with the second one including the Zarr metadata and array cache size: 500 logging statement which can be confusing for debugging purposes

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 a second round of code and functional review on the latest changes:

  • the package renaming makes sense and prefixing the pixel subclasses with Zarr will definitely help as the ecosystem of pixel buffers grows

  • the overriding of getSeries should allow us to get rid of the omero-ms-image-region implementation of PixelsService

Functionally tested in a server with a local build of this PR

  • confirmed that the lazy initialization should address the second comment of Allow the PixelsService to be runtime configured #5 (review)
  • after setting omero.pixeldata.pixels_service to /OMERO/Pixels buffer, confirmed that the thumbnail of OME-Zarr datasets is black either at import time or while updating the rendering settings
  • after unsetting omero.pixeldata.pixels_service, confirmed that the thumbnail of OME-Zarr datasets is properly generated either at import time or after updating the rendering settings

Next step is to get this integrated and combined together with glencoesoftware/omero-ms-core#31 in the image-region micro-service to test the PixelsService extensibility mechanism

@sbesson sbesson merged commit a7d5f3b into glencoesoftware:master May 3, 2024
2 checks passed
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.

2 participants