8000 Add extinfo command by dominikl · Pull Request #167 · ome/omero-cli-zarr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add extinfo command #167

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Add extinfo command #167

wants to merge 11 commits into from

Conversation

dominikl
Copy link
Member
@dominikl dominikl commented Feb 25, 2025 •
edited
Loading

This command will automatically set the ExternalInfo for ome.zarr images imported using the omero-zarr-pixel-buffer, if the --set option is specified. Default is to just print the ExternalInfo.

I takes the path from the Fileset "usedFiles", which is assumed to only have one entry ..../SOME.ome.zarr/OME/METADATA.ome.xml and then set the ExternalInfo lsid to the full path of the multiscale 5d image; ..../SOME.ome.zarr/0 for an image ..../SOME.ome.zarr/A/1/1 etc. for plates.

Limitations: Only works for ome.zarr imported from the filesystem.

Get the ExternalInfo path of an ome.zarr image.

Use --set to set the external info path
or --reset in order to remove the ExternalInfo from the image.

Positional Arguments:
  object                   Object in Class:ID format

Optional Arguments:
  In addition to any higher level options

  -h, --help               show this help message and exit
  --set                    Set the ExternalInfo path
  --lsid LSID              Use a specific lsid (path) (default: Determine from clientPath) (only used in combination with --set)
  --entityType ENTITYTYPE  Use a specific entityType (default: com.glencoesoftware.ngff:multiscales) (only used in combination with --set)
  --entityId ENTITYID      Use a specific entityId (default: 3) (only used in combination with --set)
  --reset                  Removes the ExternalInfo

@dominikl dominikl force-pushed the extinfo branch 2 times, most recently from 03bde3a to 5f3f675 Compare February 28, 2025 13:30
@joshmoore
Copy link
Member

💯 As a part of this I can imagine wanting to get and to reset as well.

@will-moore
Copy link
Member 
$ omero login
Error loading: /Users/wmoore/Desktop/ZARR/omero-cli-zarr/src/omero/plugins/zarr.py
Traceback (most recent call last):
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/cli.py", line 1677, in loadpath
    exec(f.read(), loc)
  File "<string>", line 1, in <module>
  File "/Users/wmoore/Desktop/ZARR/omero-cli-zarr/src/omero_zarr/cli.py", line 32, in <module>
    from .extinfo import external_info_str, get_extinfo, get_images, set_extern
8000
al_info
  File "/Users/wmoore/Desktop/ZARR/omero-cli-zarr/src/omero_zarr/extinfo.py", line 92, in <module>
    ) -> tuple[ImageWrapper, str | None, int | None]:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'

@dominikl
Copy link
Member Author
dominikl commented Mar 3, 2025

Interesting... is this a python version thing? I use 3.12.9.

@will-moore
Copy link
Member

It could be, I'm on an old env python 3.9.15.

@dominikl
Copy link
Member Author
dominikl commented Mar 3, 2025

Yes, looks like it's supported from 3.10 on. As we still recommend 3.9 I'll remove that stuff again.

will-moore
will-moore reviewed Mar 3, 2025
View reviewed changes
src/omero_zarr/extinfo.py Outdated
select e from ExternalInfo as e
where e.id = :id
"""
extinfo = conn.getQueryService().findByQuery(query, params)
Copy link
Member

Choose a reason for hiding this comment

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

In think this needs to be a cross-group query findByQuery(query, params, conn.SERVICE_OPTS) otherwise you won't see it if the extinfo isn't in your default group.

Copy link
Member

Choose a reason for hiding this comment

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

This is still outstanding. Same for the other conn.getQueryService().findByQuery below too.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I do that one line above conn.SERVICE_OPTS.setOmeroGroup("-1") , is that not enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, then you have to pass conn.SERVICE_OPTS that into the function call again. Ok, i'll fix that.

will-moore
will-moore reviewed Mar 3, 2025
View reviewed changes
will-moore
will-moore reviewed Mar 7, 2025
View reviewed changes
src/omero_zarr/cli.py
@@ -108,6 +109,12 @@

POLYGONS_HELP = """Export ROI Polygons on the Image or Plate in zarr format"""

EXTINFO_HELP = """Get the ExternalInfo path of an ome.zarr image.
Copy link
Member

Choose a reason for hiding this comment

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

Get or set...

Copy link
Member Author

Choose a reason for hiding this comment

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

Get, because that's the default action.

@will-moore
Copy link
Member

I was trying to test this on a non-zarr image, by specifying the --path but...

$ omero zarr extinfo Image:2856 --set --path test_extinfo_path
Previous session expired for will on localhost:4064
Server: [localhost:4064]
Username: [will]
Password:
Created session for will@localhost:4064. Idle timeout: 10 min. Current group: Swedlow Lab
Failed to set external info for image (2856) 438CTR_01_4_R3D_D3D.dv_1: Doesn't seem to be an ome.zarr: Users/wmoore/Desktop/images/DVs/438CTR_01_4_R3D_D3D.dv

It seems that there's something strange about --path, like it's being swallowed / overwritten somehow so that args.path was always None. Renaming this argument to anything else seemed to fix it. I suggest using lsid since that corresponds to the field in the extInfo.

@dominikl
Copy link
Member Author
dominikl commented Mar 7, 2025

There's a check if it is a ome.zarr: https://github.com/ome/omero-cli-zarr/pull/167/files#diff-2d64f99479103ff3050c633344ddf895f61491094e9dab65218a3ec17cbfcb59R177 As that's actually only intended to be used with zarr. Otherwise it wouldn't make sense to have it as omero-cli-zarr plugin!?

@will-moore
Copy link
Member

The point is that you have a --path argument for...

help="Use a specific path (default: Determine from clientPath) (only used in combination with --set)",

but it doesn't work for me. Does it work for you?

@dominikl dominikl force-pushed the extinfo branch 11 times, most recently from 3cc6c05 to ad4a5b2 Compare March 11, 2025 15:56
joshmoore
joshmoore reviewed Mar 11, 2025
View reviewed changes
.pre-commit-config.yaml Outdated
hooks:
- id: flake8
additional_dependencies: [
flake8-blind-except,
Copy link
Member

Choose a reason for hiding this comment

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

Too re-enable, we'll probably need help from @will-moore on knowing what exceptions can be thrown.

joshmoore
joshmoore reviewed Mar 11, 2025
View reviewed changes
.pre-commit-config.yaml Outdated
flake8-builtins,
flake8-rst-docstrings,
flake8-logging-format,
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it broke in Python 3.12: pytest-dev/pytest-testinfra#765

Copy link
Member

Choose a reason for hiding this comment

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

Over at https://github.com/ome/omero-cli-zarr/pull/168/files#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9R56 I fixed this by including setuptools as dependency.
That is now conflicting with this PR.
I wonder if you could review that PR @dominikl or @joshmoore and we can get it merged (if that fix looks good)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I temporarly close this PR and then rebase it after #168 is merged, would that work?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to close this one. Just merge in origin/master (and fix conflicts) once #168 is merged

@dominikl dominikl marked this pull request as ready for review March 11, 2025 16:52
@dominikl
Copy link
Member Author

Thanks Josh for fixing the precommit stuff! So this now suppports single image ome.zarrs, multi-series zarrs, and plates. Did I forget another use case?

@will-moore
Copy link
Member

Thanks. A few flake8 and mypy failures at https://github.com/ome/omero-cli-zarr/actions/runs/15108557228/job/42462569460?pr=167

@dominikl
Copy link
Member Author

Changing to draft PR. Probably not gonna be merged, because this functionality will have to go somewhere else (possible omero import...).

@dominikl dominikl marked this pull request as draft May 22, 2025 12:35
@dominikl dominikl force-pushed the extinfo branch 2 times, most recently from e9aff8d to a88d9b8 Compare May 23, 2025 12:53
@jburel
Copy link
Member
jburel commented May 23, 2025 •
edited
Loading

cf a88d9b8 I am not keen on the fake option. I think we should focus on creating an XML file.

@dominikl
Copy link
Member Author

I'd just need it to test a zarr plate. Setting up a fake file for it would be much quicker/easier than a full metadata.ome.xml.

@jburel
Copy link
Member
jburel commented May 23, 2025 •
edited
Loading

it is an easy way to get data in but the metadata in the xml file are then read and handle by the XML reader stored accordingly. Such support does not exist in the fake reader. It will be preferable to handle as much metadata as possible and not look at a quick access in my opinion

@jburel
Copy link
Member
jburel commented May 26, 2025

Quick discussion this am post standup: We will look at the Python snippet to generate the ome-xml file see https://github.com/IDR/idr0167-li-cellcyclenet/blob/main/scripts/ome-zarr-to-ome-xml.py

@will-moore will-moore mentioned this pull request May 26, 2025
Merged
@dominikl dominikl force-pushed the extinfo branch 6 times, most recently from f9c911a to b558927 Compare May 29, 2025 14:37
@dominikl
Copy link
Member Author

The extinfo command can now be used for different scenarios:

  • Import a zarr with metadata.xml which is part of the zarr.
    omero import -l /tmp/reader.txt ... /data/testing/plate.ome.zarr/OME/METADATA.ome.xml
    omero zarr extinfo --set Plate:1234 (it'll get the paths to set for lsid from the clientpath)

  • Import a metadata.xml from anywhere and then point to the actual zarr:
    omero import -l /tmp/reader.txt ... /tmp/some_plate_metadata.xml
    omero zarr extinfo --set --zarrPath /data/testing/plate.ome.zarr Plate:10667

Same for image zarrs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshmoore joshmoore joshmoore left review comments

@will-moore will-moore will-moore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dominikl @joshmoore @will-moore @snoopycrimecop @jburel
0