-
Notifications
You must be signed in to change notification settings - Fork 35
getObject() and getExternalInfo() load externalInfo #453
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
Conversation
559333d to
40028fa
Compare
40028fa to
295d948
Compare
|
@jburel I added support for: so we can set |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#350. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#351. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#352. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#353. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#354. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#355. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#356. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build OMERO-python-superbuild-push#357. See the console output for more details.
|
dominikl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me.Thanks Will, useful command!
sbesson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to omero.gateway to fetch the externalinfo is self-contained and allows this metadata to be exposed in the OMERO.web client as well as in the OMERO.web API.
Performance-wise, I understand we have seen no degradation although it might be worth testing in the context of a graph like a Plate?
The new command is an interesting starting point to be able to interact with these objects in command line. The current implementation raises a few design questions listed below which should be addressed before this can be merged. If it was valuable to get the gateway changes in sooner, this could be split into two separate PRs.
-
as this PR allow to set the external info associated with any object, do we need/want a mirroring functionality to get it? One possibility would be to leverage the
omero obj getfunctionality:% ./venv/bin/omero obj get ExternalInfo:799 Using session for [email protected]:4064. Idle timeout: 10 min. Current group: Public entityId=3 entityType=com.glencoesoftware.ngff:multiscales id=799 lsid=/bia-idr/S-BIAD1483/Experiment/P30/WT/Animal_1/Image_18.ome.zarr/media/taz/DATA2/Final_Tif/P30/WT/Animal_1/Image_18.ome.zarr/0 uuid=In that case, the missing link is to retrieve the ID of the ExternalInfo associated with any object. Could this be another output of
omero obj get? -
ExternalInfois immutable so callingomero ext-info-set <Object>:<id>with different properties will not update the attributes. Either the command needs to handle this or it should fail with an informative error message suggesting to delete the existing external info first (viaomero delete) -
given
ExternalInfois an object with defined attribute names unlike map, should the command use the same semantics asomero obj updateoromero obj newi.e.omero obj ext-info-set <Object>:<id> <field>=<value> [<field>=<value> ...]?
|
@sbesson Thanks for the review. Re: performance - for a Plate that contains lots of other objects, the ExternalInfo is only loaded for the root Plate object, not all the Wells, Images etc. So, performance is not an issue. But I guess this raises the question: "should we load ExternalInfo" for EVERY object that is loaded in the graph? This would be a lot more work and I'm not sure if it's needed? In the various graph-loading queries we have, we make a judgement call on how much of the graph to load, and this is just another of those questions. For getting the extInfo, how about we support: The pair of new methods I guess we could also support getting individual properties: Calling omero ext-info-set : with different properties won't try to update the immutable ExternalInfo object, but will create a new ExternalInfo with those updated properties. I'm not sure we need to support If this looks good, I'll implement |
|
Implementing some equivalent get method would certainly help assessing this PR.
You're correct. What confused me during the initial code review is that |
|
@sbesson I think I was influenced by ome/omero-cli-zarr#167 which allows setting of just the |
|
@sbesson I'm actually not sure what should be mandatory / optional. I agree that I don't know about I'm kinda assuming you got these attributes mixed up above "In that case, both entityType and entityId should be mandatory with lsid (and uuid) optional." ?? |
My suggestion was based on the
In the case of
No we cannot assume that |
|
OK, understood. |
sbesson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the last changes, getting and setting the external info on an object works as expected.
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj new Project name=test
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
No ExternalInfo found: Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
usage: ext-info-set OBJ entityId entityType [lsid] [uuid]
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1 0 mytype
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=1
entityId=0
entityType=mytype
lsid=
uuid=
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1 0 mytype mylsid
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=2
entityId=0
entityType=mytype
lsid=mylsid
uuid=
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1 0 mytype mylsid myuuid
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=3
entityId=0
entityType=mytype
lsid=mylsid
uuid=myuuid
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1 0 mytype
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Project:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Project:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=4
entityId=0
entityType=mytype
lsid=
uuid=
Two minor issues found during testing:
- the sequence of testing above results in a certain number of orphaned objects. In the case of
omero obj ext-info-set, should the command also delete previous external info after the update service call if applicable?sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero hql 'select e from ExternalInfo e' -q # | Class | Id | details | entityId | entityType | lsid | uuid ---+---------------+----+-----------------+----------+------------+--------+-------- 0 | ExternalInfoI | 1 | owner=0;group=0 | 0 | mytype | | 1 | ExternalInfoI | 2 | owner=0;group=0 | 0 | mytype | mylsid | 2 | ExternalInfoI | 3 | owner=0;group=0 | 0 | mytype | mylsid | myuuid 3 | ExternalInfoI | 4 | owner=0;group=0 | 0 | mytype | | (4 rows) - passing an invalid
entityIdfails with a non-user friendly error message. Could this be caught when casting the input toint?sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Project:1 invalidid mytype Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system Traceback (most recent call last): File "/Users/sbesson/Documents/GitHub/omero-py/./venv/bin/omero", line 8, in <module> sys.exit(main()) ^^^^^^ File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/main.py", line 125, in main rv = omero.cli.argv() ^^^^^^^^^^^^^^^^ File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/cli.py", line 1771, in argv cli.invoke(args[1:]) File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/cli.py", line 1208, in invoke stop = self.onecmd(line, previous_args) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/cli.py", line 1285, in onecmd self.execute(line, previous_args) File "/Users/sbesson/Documents/GitHub/omero-py/venv/lib/python3.12/site-packages/omero/cli.py", line 1367, in execute args.func(args) File "<string>", line 652, in process File "<string>", line 268, in go File "<string>", line 289, in on_go ValueError: invalid literal for int() with base 10: 'invalidid'
|
@sbesson Thanks - addressed those 2 points |
sbesson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Image:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
No ExternalInfo found: Image:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Image:1 invalid mytpe
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
entityId must be an integer, got: invalid
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Image:1 0 mytype
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Image:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Image:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=1
entityId=0
entityType=mytype
lsid=
uuid=
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Image:1 1 mytype
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Image:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Image:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=2
entityId=1
entityType=mytype
lsid=
uuid=
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-set Image:1 1 mytype mylsid myuuid
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
Image:1
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero obj ext-info-get Image:1
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
id=3
entityId=1
entityType=mytype
lsid=mylsid
uuid=myuuid
sbesson@Sebastiens-MacBook-Pro-3 omero-py % ./venv/bin/omero hql 'select e from ExternalInfo e'
Using session for root@localhost:4064. Idle timeout: 10 min. Current group: system
# | Class | Id | details | entityId | entityType | lsid | uuid
---+---------------+----+-----------------+----------+------------+--------+--------
0 | ExternalInfoI | 3 | owner=0;group=0 | 1 | mytype | mylsid | myuuid
(1 row)
To see details for object, enter line number.
To move ahead one page, enter 'p'
To re-display list, enter 'r'.
To quit, enter 'q' or just enter.
q
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/compatibility-of-ome-zarr-stored-on-s3-and-omero/111380/17 |
See ome/omero-web#613.
This tweaks the base
BlitzObjectWrapper._getQueryString()to include loadingexternalInfo.It also adds
BlitzObjectWrapper.getExternalInfo()to return theexternalInfoand load it if necessary, e.g. for cases whereself._objis NOT the object loaded bygetObject().E.g. for image:
To test:
Set some values for ExternalInfo:
Using this branch, we can set
EntityId, EntityType(both required) andLsid, UUid(optional):Since the JSON API uses
_getQueryString()to build queries, and omero-marshal already handles externalInfo, you can check the details at E.g./api/v0/m/images/ID/: (NB: deployed on idr-testing, e.g./api/v0/m/images/15159664/).