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

Initial feedback #2

Open
joshmoore opened this issue Jun 24, 2020 · 8 comments
Open

Initial feedback #2

joshmoore opened this issue Jun 24, 2020 · 8 comments

Comments

@joshmoore
Copy link
Collaborator

Hi @tlambert03.

I gave napari-omero a try yesterday. Very nice! I'm bundling together a couple of issues I ran into for general discussion, but I'm happy to break them out into separate issues moving forward.

  • I ran into an exception pointing to SessionsStore.ctx if I didn't pre-login from the CLI. This looks to be an assumption somewhere deeper in the omero-py code, but trying to mix pdb and Qt was less than successful. Do you always have an active login when running? This may be related to the security settings of IDR, but I don't have a clear smoking gun yet.

  • I also note that a login appears to attempted before the user hits connect. I assume this is an attempt to re-use the previous session from session store? As a heads up, the CLI does this (and you've clearly drilled down pretty far into that code) but no other client does. It'd be interesting to hear your feedback.

  • When running the project/dataset browser against the IDR, my instance seemed to go into an infinite loop. I haven't started to track this one down yet.

  • Large images explode. I assume this would require loading tile by tile. I don't know if you have any plans to support that at the moment.

cc: @will-moore

@tlambert03
Copy link
Owner

Thanks a lot for the feedback!

exception pointing to SessionsStore.ctx ... Do you always have an active login when running?

handling of the client/gateway is definitely the thing I could use the most help with. This one likely calls for a separate issue, so I'll create one and give a partial answer there.

I also note that a login appears to attempted before the user hits connect. I assume this is an attempt to re-use the previous session from session store? As a heads up, the CLI does this but no other client does.

Yeah, I was mostly just looking for a way not to have to enter my password every time! 😂. I kinda like the behavior, but I'm definitely open to feedback on that. If you think it's security issue, we could make it opt-in somehow. At the very least, we need a "logout" button... though that is made slightly more complicated by the fact that the lazy dask arrays require the connection, so we'd basically need to either delete all currently-loaded layers with dask data (or convert them to local arrays) in order to avoid crashes after logging out.

When running the project/dataset browser against the IDR

I've never really used it... is that something I can try without a login? or can I create a login?

Large images explode. I assume this would require loading tile by tile.

def... it's a disaster at the moment. Yeah that would require tiling, so A) the dask chunk size here would need to be something other than the full XY plane and B) napari would need better 2D tiling support. Can the omero pixels API get a chunk/tile less than the full plane? or does that require something like zarr on the backend? as for tiling/chunking support in napari @pwinston is working hard on that sort of thing at the moment. So it's definitely a future goal.

@will-moore
Copy link
Collaborator

I've looked into supporting 'Large images' at #1
I create a dask delayed loader for each tile and concatenate them into each plane.
But OMERO rawPixelsStore doesn't support pyramids, so I've had to use the rendering engine. So you get a 'rgb' image with the current OMERO rendering settings. Work in progress...

@tlambert03
Copy link
Owner

oh i see. very cool.

@joshmoore
Copy link
Collaborator Author

But OMERO rawPixelsStore doesn't support pyramids,

See #1 (comment) ?

@joshmoore
Copy link
Collaborator Author

Yeah, I was mostly just looking for a way not to have to enter my password every time! 😂. I kinda like the behavior, but I'm definitely open to feedback on that. If you think it's security issue, we could make it opt-in somehow.

I don't think it's a particular security issue. You've followed the CLI model (which I support: I'm to blame if anything is amiss there) but it's different from other GUIs, so just raising the flag.

@tlambert03
Copy link
Owner

Ok good to know. It’s an easy pattern to change in the future, so I’ll leave it for the time being

@tlambert03
Copy link
Owner

tlambert03 commented Jun 25, 2020

I ran into an exception pointing to SessionsStore.ctx if I didn't pre-login from the CLI. This looks to be an assumption somewhere deeper in the omero-py code

@joshmoore, looks like this is because I create a SessionStore here but do not provide it with a ctx attribute. however, in the SessionsStore.create method in omero-py, it assumes that a ctx has been injected into the class somewhere else in the code. Seems like it could be fixed on both ends... should I submit that to omero-py?

@joshmoore
Copy link
Collaborator Author

Those self.ctx. items look very much like a borked refactoring. If you have a fix, a PR against omero-py would be great.

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

No branches or pull requests

3 participants