Add more opts support for getObjects('Annotation')

[will-moore]

conn.getObjects("Annotation")

This adds support for various options for conn.getObjects("Annotation", opts), in preparation for JSON api:
The OMERO_CLASS is now set on AnnotationWrapper objects.
Also Fixes #433.

This is used by ome/omero-web#682

Tests added in ome/openmicroscopy#6458

Supported opts - All are optional (but parent_ids needs parent_type)

• "parent_type": "dataset"
• "parent_ids": [1, 2]
• "ns": "my.namespace"

E.g. /api/tagannotations/?project=1&project=2 would be backed by:

conn.getObjects("TagAnnotation", opts={
  "parent_type": "project",
  "parent_ids": [1,2]
})

%sAnnotationLink

This also improves the formulation of %sAnnotationLink (E.g. ImageAnnotationLink, ExperimenterGroupAnnotationLink, AnnotationAnnotationLink) in multiple places:

• conn.getObjectsByMapAnnotations("Dataset", key="foo")
• obj._loadAnnotationLinks() / _getAnnotationLinks()
  • unlinkAnnotations()
  • removeAnnotations()
  • getAnnotation()
  • listAnnotations()
• obj._linkAnnotation() / _linkObject unused?
• conn.getAnnotationLinks()
• conn.countAnnotations()
• conn.listOrphanedAnnotations()
• AnnotationWrapper._getQueryString()

We can now do getObject() and listAnnotations() etc with many more object types:

E.g:

tag = omero.gateway.TagAnnotationWrapper(conn)
tag.setValue("Test Tag on Group")

group = conn.getGroupFromContext()
group.linkAnnotation(tag)

group.listAnnotations()

Known issue: Loading Parents

For webclient/api/annotations/ we get the annotations along with links to the parents and the parent objects too.
E.g.

  "link": {
    "id": 39752336,
    "owner": {
      "id": 2
    },
    "parent": {
      "id": 3414011,
      "class": "ImageI",
      "name": "10percent-Wt1-GFP-spheroid-MV.czi [0]"
    },
    "date": "2017-11-27T14:13:19+00:00",
    "permissions": {
      "canDelete": false,
      "canAnnotate": false,
      "canLink": false,
      "canEdit": false
    }
  }

This is useful to have permissions, timestamp, owner etc on the link but it is particularly essential when we are loading annotations on multiple objects. E.g. /api/annotations/?type=tag&project=1&project=2 would load annotations for 2 projects, BUT we without links, we don't know which Annotations are linked to which Project.

But, it doesn't appear possible to achieve this with omero_marshal, since for Annotations, there is no handling of links to Parent objects. Annotations are "Annotatable", so we check for annotations ON the object https://github.com/ome/omero-marshal/blob/f8ff1c2e439f0599d96423b7f64898864548ba9f/omero_marshal/encode/encoders/annotation.py#L68
but not whether the Object (annotation) is annotating a Parent.
In fact, I don't see that it's possible for e.g. an Annotation to have links loaded (in the same way that a Project or Dataset can have obj.copyAnnotationLinks(), since the possible links for an Annotation object are stored in a different Table for each parent, e.g. ProjectAnnotationLinks, DatasetAnnotationLinks etc.

[will-moore]

Tests added in ome/openmicroscopy#6458

[will-moore]

Comment on what changed after first commit....

The problem with the getObjects() is that we can't easily handle OR logic. E.g. "get Annotations with links to Project:1 OR Dataset:2". Currently, for each obj_type we add a clause, so we get AND logic:

                    clauses.append(
                        "exists (from %sAnnotationLink as link "
                        "where link.child.id = obj.id "
                        "and link.parent.id in (:%s_ids))" % (obj_type, plural))
                    params.add("%s_ids" % plural, rlist([rlong(i) for i in ids]))

Therefore,
conn.getObjects("Annotation", opts={"ann_type": "tag", "projects": [1,2], "datasets": [3,4]}) won't give you annotations on all objects, as you currently get with e.g. https://idr.openmicroscopy.org/webclient/api/annotations/?type=map&screen=3&image=3414011

Do we ever need to load annotations for more that 1 type of Object? E.g. webclient almost never allows you to select multiple different Object types, (e.g. Dataset and Image, except maybe in the search results)

The webclient/api endpoint does a series of queries select oal from %sAnnotationLink as oal for each obj_type requested. Each Database query only supports 1 object type. So it's probably OK for the getObjects("Annotation") to also support just ONE parent type. Same behaviour in E.g.

conn.getAnnotationLinks(...parent_type, parent_ids=None)

So, let's instead go for:

conn.getObjects("Annotation", opts={
  "ann_type": "tag",
  "parent_type": "project",
  "parent_ids": [1,2]
})

[knabar]

Do we ever need to load annotations for more that 1 type of Object? E.g. webclient almost never allows you to select multiple different Object types, (e.g. Dataset and Image, except maybe in the search results)

The webclient/api endpoint does a series of queries select oal from %sAnnotationLink as oal for each obj_type requested. Each Database query only supports 1 object type. So it's probably OK for the getObjects("Annotation") to also support just ONE parent type.

It makes sense to me to limit the parent type to one per request, if a client requires annotations on different parent types, the client can make multiple requests.

I think it could even be justified to limit requests to a single parent object if that simplifies the requests and responses significantly.

[will-moore]

@knabar Anything else to address here?

I think it could even be justified to limit requests to a single parent object if that simplifies the requests and responses significantly.

There is no additional logic associated with supporting multiple parent objects (IDs) of the same type, and it could be useful so I'd like to leave this in.

[sbesson]

A few initial comments. I think the extensibility to support the filtering by annotation type, parent and namespace is useful but we will need additional changes to handle more generically all relevant objects.

[sbesson]

Is there a list of all objects that can be annotated?

[will-moore]

I don't actually know where to find such a list or if it's possible to create one?

[will-moore]

https://omero.readthedocs.io/en/stable/developers/Model/EveryObject.html#annotation seems like a good start...

[sbesson]

Unfortunately, PlateAcquisition is not the only object that will need this special handling to convert from its lowercase version. Other examples include all annotation types (which can be annotated themselves), some instrument objects (LightPath, LightSource...) as well as OriginalFile.

[will-moore]

Using this list (from https://omero.readthedocs.io/en/stable/developers/Model/EveryObject.html#annotation)

AnnotationAnnotationLink
ChannelAnnotationLink
DatasetAnnotationLink
DetectorAnnotationLink
DichroicAnnotationLink
ExperimenterAnnotationLink
ExperimenterGroupAnnotationLink
FilesetAnnotationLink
FilterAnnotationLink
FolderAnnotationLink
ImageAnnotationLink
InstrumentAnnotationLink
LightPathAnnotationLink
LightSourceAnnotationLink
NamespaceAnnotationLink
NodeAnnotationLink
ObjectiveAnnotationLink
OriginalFileAnnotationLink
PlaneInfoAnnotationLink
PlateAcquisitionAnnotationLink
PlateAnnotationLink
ProjectAnnotationLink
ReagentAnnotationLink
RoiAnnotationLink
ScreenAnnotationLink
SessionAnnotationLink
ShapeAnnotationLink
WellAnnotationLink

I see only these cases:

ExperimenterGroup
LightPath
LightSource
OriginalFile
PlaneInfo
PlateAcquisition

If the parent_type is annotation then this will query for AnnotationAnnotationLink without any other handling needed.

[sbesson]

This is getting much better in particular with the removal of the duplicated _getQueryString to use OMERO_CLASS (with the additional clause for fetching OriginalFile).

Reviewing the list of structured annotations, the only concrete instantiation that misses a wrapper is ListAnnotation. Is that something we want to quickly implement as part of this PR to be feature complete?

[sbesson]

Can we move this utility to a separate utility method for transforming lower-case strings into case-sensitive object types with associated unit tests ?

I see a few places in the gateway that can use it. There might be use cases elsewhere in OMERO.py.

[will-moore]

Added test in 93eb834 to test the case-insensitive handling of object names

[pwalczysko]

@will-moore excluding - this PR is causing the test_show.py errors in https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/43/testReport/

[will-moore]

Updated the %sAnnotationLink logic in these methods:

• conn.getObjectsByMapAnnotations("Dataset", key="foo")
• obj._loadAnnotationLinks() / _getAnnotationLinks()
  • unlinkAnnotations()
  • removeAnnotations()
  • getAnnotation()
  • listAnnotations()
• obj._linkAnnotation() / _linkObject unused?
• conn.getAnnotationLinks()
• conn.countAnnotations()
• conn.listOrphanedAnnotations()
• AnnotationWrapper._getQueryString()

[will-moore]

Currently 1 test failure at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/60/

def testGetAnnotations(self, gatewaywrapper, author_testimg_tiny):
...
# Check links to Tag on the Comment
        for obj_type in ["CommentAnnotation", "Annotation"]:
>           annLinks = list(gatewaywrapper.gateway.getAnnotationLinks(
                obj_type, parent_ids=[ann.getId()]))

        wrapper = KNOWN_WRAPPERS.get(parent_type.lower(), None)
>       link_name = wrapper.ann_link_name()
                    ^^^^^^^^^^^^^^^^^^^^^
E       AttributeError: 'function' object has no attribute 'ann_link_name'

../../../../.venv3/lib64/python3.11/site-packages/omero/gateway/__init__.py:3556: AttributeError

[pwalczysko]

A new failing test - freshly from today is https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/62/testReport/OmeroPy.test.integration.gatewaytest.test_get_objects/TestGetObject/testGetObjectsAnnotation/

cls = <class 'omero.gateway.AnnotationWrapper'>
opts = {'parent_type': 'Dataset', 'parent_ids': [572]}

    @classmethod
    def _getQueryString(cls, opts=None):
        """
        Used for building queries in generic methods such as
        getObjects("Annotation")
        Returns a tuple of (query, clauses, params).
    
        :param opts:        Dictionary of optional parameters.
                            parent_type: (optional) "Project", "Dataset", "Image" etc
                            parent_ids: (optional) list of IDs for the parent type
                            ns: (optional) namespace string to filter by
        :return:            Tuple of string, list, ParametersI
        """
    
        query, clauses, params = super(
            AnnotationWrapper, cls)._getQueryString(opts)
        if opts is None:
            opts = {}
    
        fetch_file = ""
        if cls.OMERO_CLASS in ("Annotation", "FileAnnotation"):
            fetch_file = "left outer join fetch obj.file as file"
    
        query = (f"select obj from {cls.OMERO_CLASS} obj "
                 f"{fetch_file} "
                 "join fetch obj.details.owner as owner "
                 "join fetch obj.details.creationEvent")
    
        # We want to make parent_type case-insensitive...
        if 'parent_type' in opts:
>           ann_link_name = ann_link_name(opts['parent_type'])
                            ^^^^^^^^^^^^^
E           UnboundLocalError: cannot access local variable 'ann_link_name' where it is not associated with a value

[will-moore]

@sbesson ready for another review, thanks.
I updated the %sAnnotationLink logic everywhere that uses it in the BlitzGateway.
So you can now do e.g:

        tag = omero.gateway.TagAnnotationWrapper(conn)
        tag.setValue("Test")
        group = conn.getGroupFromContext()
        group.linkAnnotation(tag)

        # also link annotations
        tag.linkAnnotation(anotherAnnotation)

        # listAnnotations works too
        group.listAnnotations()

and the ExperimenterGroupAnnotationLink and AnnotationAnnotationLink objects will be created.

Tests for both of these are in PR at ome/openmicroscopy#6458

[will-moore]

@sbesson Do we have any more omero-py PR reviews in the calendar (I'm not seeing any)? Or maybe some other time we can discuss/review this PR? Thx

[sbesson]

Do we have any more omero-py PR reviews in the calendar (I'm not seeing any)?

That's correct, it looks like the monthly OMERO PR review meeting expired. I propose we discuss this at the next Tuesday meeting. In particular, with the upcoming work towards OMERO.server 5.7.0, we might want to restart them and agree about the frequence and time.

Or maybe some other time we can discuss/review this PR?

I wasn't sure about the state of this work based on the commits pushed after #489 (comment) here and in the companion PR as well as the merge conflicts raised in #489 (comment). If you are happy with its current state and consider it ready for re-evaluation, I will schedule another round of review probably next week.

[will-moore]

Those last commits were fixing additional tests I added at ome/openmicroscopy@1adbba8
and also to avoid merge conflicts with #459 (and because I'd missed getAnnotationLinkTableName() which was modified in that PR)

[sbesson]

This is looking great. The extended usage of KNOWN_WRAPPERS, the introduction of the ann_link_name convenience function and the removal of the special casing duplicated in various place is a good step forward in terms of maintenance. Same for the consolidation and the generalization of _getQueryString.

A couple of inline comments mostly around the usage of wrapper classes that were not part of the initial scope (experimenter & light sources) but can be annotated as per the model.

In addition to the integration tests, could we also expand the unit tests under test/unit/gatewaytest to have some coverage of the new ann_link_name API for all supported object types. Given the complexity of the gateway code, this will reduce the risk of future regressions.

[sbesson]

One minor inline question about the implementation of ExperimenterWrapper.linkAnnotation().

The integration tests introduced in ome/openmicroscopy#6458 should cover the APIs modified by this PR for all annotation type but were not been included in the nightly CI builds. Once we get a succesful run with the extended integration tests and unit tests are added to this PR as discussed in #489 (review) , we should be in a position to get this merged.

[knabar]

Don't have anything to add to @sbesson's comments

[sbesson]

See https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/96/testReport/OmeroPy.test.integration.gatewaytest/ and https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/96/testReport/OmeroPy.test.integration.gatewaytest.test_get_objects/ for the execution of the new integration tests matching these changes.