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 plugins to use settings utilities #513

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

chris-allan
Copy link
Member

There are various settings utilities included in the omeroweb.settings module that would be useful to use in plugins. Unfortunately, because of import order and plugin settings processing that's made impossible if they are in the same package. The LeaveUnset functionality is of particular interest. This moves them into omeroweb.utils so they can be if desired.

There are various settings utilities included in the `omeroweb.settings`
module that would be useful to use in plugins.  Unfortunately, because
of import order and plugin settings processing that's made impossible if
they are in the same package.  The `LeaveUnset` functionality is of
particular interest.  This moves them into `omeroweb.utils` so they can
be if desired.
Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Was the issue that it wasn't possible to do from omeroweb.settings import LeaveUnset because this affects the order that things are imported?
Changes look good and everything's green so 👍

@chris-allan
Copy link
Member Author

Since plugin.settings are processed during the first import of omeroweb.settings, yes, kind of. It's not that it affects the order it's just not possible at all due to the calling context and our use of __import__ in process_custom_settings(). Similar to having a modules a and b which both import things from each other where you would get something like:

ImportError: cannot import name '...' from partially initialized module 'a' (most likely due to a circular import)

I was also finding myself re-implementing parse_boolean() in almost every plugin for the same reasons.

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.

Looking at the ecosystem of third-party OMERO.web apps available through GitHub, I definitely see a series of places where settings are declaring their own utility methods and some of the upstream OMERO.web logic could likely be re-used:

https://github.com/sukunis/OMERO.openlink/blob/db48a1f2e60f0dd099d87953aa55f93cd66575df/omero-openlink/omero_openlink/openlink_settings.py#L6-L10
https://github.com/jmuhlich/omero-oauth/blob/7cce1d48a6ab3ad0f3847d1cde4918feb129ce60/omero_oauth/oauth_settings.py#L16-L19
https://github.com/ome/omero-signup/blob/c8610dba25f39870a09fa5401c735fb3be85a319/omero_signup/signup_settings.py#L9-L13

Note in a few plugins, some of the methods migrated here seem to be imported directly

https://github.com/IDR/idr-gallery/blob/f96d66c6c99ff69de0c09be6cc97996f94397671/idr_gallery/gallery_settings.py#L23C1-L24
https://github.com/ome/omero-signup/blob/c8610dba25f39870a09fa5401c735fb3be85a319/omero_signup/signup_settings.py#L3C1-L3C1

This probably brings two questions:

  • why are these plugins not having the circular dependency mentioned in Allow plugins to use settings utilities #513 (comment) ?
  • removing these functions from omeroweb.settings will cause backwards incompatible changes for all plugins importing them. If this is a concern, a middle ground might be to move the logic to omeroweb.utils as proposed but keep (and deprecate) and alias in omeroweb.settings

@chris-allan
Copy link
Member Author

why are these plugins not having the circular dependency mentioned in #513 (comment) ?

Because it's not strictly a circular dependency but rather the calling context during process_custom_settings(). The semantics are weird, I can get into them if you want but honestly I wasn't for writing a whole essay here on how the processing of settings in omeroweb.settings works and what will or will not work with which implementation details. It even depends on which server type you are using.

removing these functions from omeroweb.settings will cause backwards incompatible changes for all plugins importing them.

No it will not as we're importing all the functions that were moved to omeroweb.utils. For example, from omeroweb.settings import parse_boolean will still work with the changes proposed here.

@chris-allan
Copy link
Member Author

Same for iviewer: https://github.com/ome/omero-iviewer/blob/aac33c0b897744fe0cf12cd6ce4500f65c7db64f/plugin/omero_iviewer/iviewer_settings.py#L23

Yes, none of these examples, including those from @sbesson above, utilize the built-in CUSTOM_SETTINGS_MAPPINGS feature. There has been no need for plugin developers to perform explicit settings inclusion for a very long time but these do. I expect a bunch of plugins have been added over the years by the copy and paste method where the legacy features were used so we are where we are. The aforementioned plugins even use Django 1.x style urls.py.

@sbesson
Copy link
Member

sbesson commented Nov 16, 2023

For completeness, CUSTOM_SETTINGS_MAPPINGS is the recommended way for OMERO.web app developers to declare their own settings - https://omero.readthedocs.io/en/stable/developers/Web/CreateApp.html#app-settings so there is definitely value in fixing these imports.

@will-moore I found no OME web app using this settings layout. I assume as a proof of concept, it should be possible to update either omero-signup and/or idr-gallery settings to use CUSTOM_SETTINGS_MAPPINGS. I suspect that without this change, the omeroweb.settings import will fail as described by @chris-allan in #513 (comment) but with this OMERO.web PR included, the settings should be functional.

@knabar knabar added this to the 5.24.0 milestone Nov 17, 2023
@knabar knabar merged commit 8bcbf20 into ome:master Dec 12, 2023
8 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.

4 participants