Python gateway: ExternalInfo getter regression

[sbesson]

In OMERO.py 5.21.0, #453 introduced a new getExternalInfo API allowing to lazy-load ExternalInfo objects available from the details of various OMERO model objects.

The addition of this API caused some regression that can be demonstrated in the following example:

import omero.gateway
conn = omero.gateway.BlitzGateway(host=host,username=username,passwd=password)
try:
    conn.connect()
    image = conn.getObject("Image", iamge_idid)
    ext_info = image.getDetails().getExternalInfo()
finally:
    conn.close()

With OMERO.py 5.20.0 executing the above against a running server and valid image ID works but with OMERO.py 5.21.0 or 5.21.1 it will fail with the following stack trace

Traceback (most recent call last):
  File "/Users/sbesson/Documents/GitHub/omero-py/test.py", line 7, in <module>
    ext_info = image.getDetails().getExternalInfo()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/gateway/__init__.py", line 1355, in getExternalInfo
    extinfo = self.getDetails()._externalInfo
              ^^^^^^^^^^^^^^^^^
  File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/gateway/__init__.py", line 472, in getDetails
    if self._obj.loaded:
       ^^^^^^^^^^^^^^^^
  File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero_model_DetailsI.py", line 106, in __getattr__
    raise AttributeError(attr)
AttributeError: loaded

The underlying issue is that getExternalInfo API was defined at the BlitzObjectWrapper level and is inherited by all instance wrappers. This includes DetailsWrapper and overrides the previous getter which was either returning None or a BlitzObjectWrapper object depending on the use case.

One way to fix this is to override getExternalInfo again in DetailsWrapper restoring the previous behavior. @will-moore do you know if there is a better option to limit the getExternalInfo API to the relevant wrappers?

[will-moore]

Ah, thanks for finding that.
I don't think there's a better option that adding getExternalInfo() to the DetailsWrapper class for backwards compatibility.
Although I expect new code should simply do image.getExternalInfo() now.

[sbesson]

While writing unit tests, I found another discrepancy. ImageWrapper.getExternalInfo() returns the unwrapped omero.model.ExternalInfo object while DetailsWrapper.getExternalInfo() returned a wrapped BlitzObjectWrapper object.
Was this on purpose?

[will-moore]

No, I didn't actually consider wrapping the externalInfo in ImageWrapper.getExternalInfo().
Hmmm... I guess that really should be wrapped.

Do we consider that a bug and simply fix it. Or would that be considered a breaking API change?
It's not been released long and unlikely to be consumed by many.

[sbesson]

It is backwards incompatible since anyone who would have consumed the API introduced in OMERO.py 5.21.0 might need to fix their code. For instance image.getExternalInfo().getLsid() currently returns an rtype. If the type was changed to a BlitzObjectWrapper this would automatically unwrap it into a string.

To fix this without breaking, I can see two routes

1. add some form of wrapped={True,False} flag and leave the default to False
2. deprecate BlitzObjectWrapper.getExternalInfo() and replace it by another API that returns the wrapped object

[will-moore]

• image.getExternalInfo(wrapped=True) would work, but doesn't feel very nice.
• image.getExtInfo() or image.getExternalInfo2() could be a bit confusing alongside image.getExternalInfo().

The problem with both of those solutions is that they will be around forever, so we will always be suffering my original error. They are not a significant improvement on the current state. In all cases, the API doesn't behave quite as you'd expect.

The only solution to achieve that is to make a breaking change (in 5.22.0). Users would need to make a small fix and know to avoid versions 5.21.x. I know we strive to avoid breaking changes, but I feels it's worth considering in this case given the small impact.

Out of the 4 options, I don't have a strong preference. All have their pros and cons.

• 1 wrapped agument
• 2 replace with another method
• 3 no change
• 4 breaking change

[sbesson]

I opened #482 to propose a resolution of the regression as this should really be the priority. Note this is preserving the current statu quo regarding the contract of the API introduced in OMERO.py 5.21.0 i.e. in practice it is following option 3 listed above for now.

[will-moore]

Thanks, PR looks good.

Since 5.21.0 and 5.21.1 have broken image.getDetails().getExternalInfo(), it doesn't seem a big leap to consider that they also have broken image.getExternalInfo(). In any case we would tell users to update to 5.22.0.

So, I have a slight preference for option 4 (but only if we do it now)!

[sbesson]

@will-moore trying to take a step back, is there any functionality that the ImageWrapper.getExternalInfo() API offers that is not covered by ImageWrapper.getDetails().getExternalInfo() ?

[will-moore]

No, I don't think so

[sbesson]

In that case, would a fifth option be to deprecate BlitzObjectWrapper.getExternalInfo() and recommend any potential adopters to switch their calls to use BlitzObjectWrapper.getDetails().getExternalInfo() which lazy loads and wraps the ExternalInfo consistently with the rest of the gateway?

[will-moore]

Ah - @sbesson I realise I was wrong...

ImageWrapper.getExternalInfo() will load the details on the fly if not loaded - see

omero-py/src/omero/gateway/__init__.py

Line 1356 in 780876c

if extinfo is not None and not extinfo._loaded:

So with this code:

        dataset = conn.getObject("Dataset", 251)

        for image in dataset.listChildren():
            extInfo = image.getExternalInfo()
            print(unwrap(extInfo.entityId))

            extInfo = image.getDetails().getExternalInfo()
            print(extInfo.entityId)

I get:

  File "/Users/wmoore/Desktop/python-scripts/externalInfo_test.py", line 23, in main
    extInfo = image.getDetails().getExternalInfo()
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/wmoore/opt/anaconda3/envs/omeroweb_zarrv3/lib/python3.12/site-packages/omero/gateway/__init__.py", line 1355, in getExternalInfo
    extinfo = self.getDetails()._externalInfo
              ^^^^^^^^^^^^^^^^^
  File "/Users/wmoore/opt/anaconda3/envs/omeroweb_zarrv3/lib/python3.12/site-packages/omero/gateway/__init__.py", line 472, in getDetails
    if self._obj.loaded:
       ^^^^^^^^^^^^^^^^
  File "/Users/wmoore/opt/anaconda3/envs/omeroweb_zarrv3/lib/python3.12/site-packages/omero_model_DetailsI.py", line 106, in __getattr__
    raise AttributeError(attr)

[will-moore]

Ah - I realise that overwriting getExternalInfo() in 5.21.0 also broke the lazy loading...
With 5.20.0 this works, even though externalInfo isn't loaded:

        for image in dataset.listChildren():
            details = image.getDetails()
            if details.externalInfo is not None:
                print("details loaded?", details._externalInfo._loaded)

                extInfo = details.getExternalInfo()
                if extInfo is not None:
                    print(extInfo.entityId)

[will-moore]

Re: option 5 - I would prefer to fix image.getExternalInfo() than to deprecate it. I see the current behaviour as a bug that needs fixing, rather than an API we have stopped supporting.