Add more opts support for getObjects('Annotation') - #489
Conversation
|
Tests added in ome/openmicroscopy#6458 |
|
Comment on what changed after first commit.... The problem with the Therefore, 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
So, let's instead go for: |
We may be loading file annotations even without using ann_type of 'file'
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. |
|
@knabar Anything else to address here?
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
left a comment
There was a problem hiding this comment.
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.
| elif ann_type == "map": | ||
| clauses.append("obj.class=MapAnnotation") | ||
| elif ann_type is not None: | ||
| msg = ("ann_type '%s' not recognised. Must be one of 'tag', 'file', " |
There was a problem hiding this comment.
This should be implemented for all supported annotations (BooleanAnnotation, TermAnnotation, XmlAnnotation...)
|
|
||
| :param opts: Dictionary of optional parameters. | ||
| NB: No options supported for this class. | ||
| ann_type: (optional) "tag", "file", "comment", "long", "map" |
There was a problem hiding this comment.
Arguably this could use the full annotation type TagAnnotation, FileAnnotation and save the mapping below.
There was a problem hiding this comment.
Do you mean "TagAnnotation" (string) or omero.model.TagAnnotation (class)?
I feel that it's more user-friendly for the BlitzGateway methods to take strings rather than omero.model objects. E.g. conn.getObject("Image") rather than conn.getObject(omero.model.ImageI).
So I'd like to do the mapping here. Otherwise we'd have to do the mapping in omero-web for ome/omero-web#682 and probably other places too.
I have just realised that the more natural way to do this (since we only support a single ann type) is to do:
e.g. get Tags on datasets... NB: we already support conn.getObject("TagAnnotation")
conn.getObject("TagAnnotation", opts={"parent_type": "dataset", "parent_ids": [1, 2]})
instead of
conn.getObject("Annotation", opts={"ann_type": "tag", "parent_type": "dataset", "parent_ids": [1, 2]})
There was a problem hiding this comment.
Agreed, that's a more natural extension of the existing API and substantially simplifies the logic.
I think it would make sense to use the same semantics for the parent_type e.g. when retrieving all the tags under a tagset, one could use
conn.getObjects("TagAnnotation", opts={"parent_type": "TagAnnotation", "parent_ids": [1], ns: omero.constants.metadata.NSINSIGHTTAGSET})
This would remove the special casing discussed in #489 (comment)
There was a problem hiding this comment.
Actually, conn.getObjects("dataset') or conn.getObjects("tagannotation") is already case-insensitive, so lets stick with that here too for consistency.
| :param opts: Dictionary of optional parameters. | ||
| NB: No options supported for this class. | ||
| ann_type: (optional) "tag", "file", "comment", "long", "map" | ||
| parent_type: (optional) "project", "dataset", "image" etc |
There was a problem hiding this comment.
Is there a list of all objects that can be annotated?
There was a problem hiding this comment.
I don't actually know where to find such a list or if it's possible to create one?
There was a problem hiding this comment.
https://omero.readthedocs.io/en/stable/developers/Model/EveryObject.html#annotation seems like a good start...
| raise AttributeError(msg) | ||
|
|
||
| if 'parent_type' in opts: | ||
| obj_type = opts['parent_type'].title().replace("Plateacquisition", "PlateAcquisition") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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?
| if obj_type == otype.title(): | ||
| obj_type = otype | ||
| ids_clause = "" | ||
| if 'parent_ids' in opts: |
There was a problem hiding this comment.
What is the real-world use case associated with specifying parent_type but no parent_ids?
I think it should be easy to enforce that parent_ids must be supplied if parent_type is specified. And that can always be relaxed later on if there is a real need.
|
|
||
| # We want to make parent_type case-insensitive... | ||
| if 'parent_type' in opts: | ||
| # Title case works for most objects... |
There was a problem hiding this comment.
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 excluding - this PR is causing the |
|
More specifically, the addition of omero-py/src/omero/gateway/__init__.py Lines 3551 to 3554 in 5d0974d |
This adds support for various options for
conn.getObjects("Annotation", opts), in preparation for JSON api: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]"ann_type': "tag""ns": "my.namespace"E.g.
/api/annotations/?type=tag&project=1&project=2would be backed by: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.
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=2would 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#L68but 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, DatasetAnnotationLinksetc.