Skip to content
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

made Importer `zarr´ compatible #99

Merged

Conversation

jo-mueller
Copy link
Contributor

Description

Fixes #97

This PR implements the fixes suggested by @will-moore in this image.sc thread to allow for the import of .zarr files into OMERO through ezomero.

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.

@@ -468,6 +470,7 @@ def ezimport_ln_s(self) -> bool:
'-k', self.conn.getSession().getUuid().val,
'-s', self.conn.host,
'-p', str(self.conn.port),
'--depth', '10',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to suggest that, instead of hardcoding that, ezimport can take extra arguments that get passed through to the import call at CLI level. This way, if we ever need --depth 20, or if folks need to pass any other optional import arguments, that doesn't require any code change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erickmartins thanks for the feedback. Do you mean adding it as a keyword argument to the Importer class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean adding *args, **kwargs to the ezimport function signature, and passing these through to the omero import call there (changing imp_ctl.ezimport() to imp_ctl.ezimport(*args, **kwargs)). In fact, that would also mean getting rid of the ln_s optional parameter and the ezimport_ln_s method.

Copy link
Collaborator

@erickmartins erickmartins Mar 20, 2024

Choose a reason for hiding this comment

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

db4f409 is more or less what I had in mind here - it means users can pass extra arguments directly into ezimport and they get sent to omero import. in that case, making it work for zarrs would just mean calling ezimport(conn, "/path/to/file.zarr", depth=10), doing in-place imports would just be calling ezimport(conn, "/path/to/file.tiff", transfer="ln_s") and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for working on this! I'm on parental leave until early April so thanks big-time for the fix :)

@erickmartins erickmartins merged commit b707ab4 into TheJacksonLaboratory:main Apr 1, 2024
1 check passed
@jo-mueller jo-mueller deleted the zarr-compatibility branch April 4, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ezimport doesn't support OME-NGFF
2 participants