jni / zarpaint

Paint segmentations directly to on-disk/remote zarr arrays
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

Warn at import time if tensorstore is missing #47

Closed GenevieveBuckley closed 10 months ago

GenevieveBuckley commented 10 months ago

Closes https://github.com/jni/zarpaint/issues/46

This PR adds a warning at import time if the optional tensorstore dependency is missing, since the performance of zarpaint is much faster when tensorstore is available.

Here is the output users see:

In [1]: import zarpaint
/Users/genevieb/Documents/GitHub/zarpaint/src/zarpaint/__init__.py:6: UserWarning:
zarpaint performance is much faster if tensorstore is also installed.
It is recommended to install tensorstore with:
python -m pip install tensorstore

  warnings.warn(

In [2]: 
jni commented 10 months ago

@GenevieveBuckley zarpaint does have utilities that work without tensorstore or zarr, so I kinda think I would prefer to defer the warning until tensorstore is used... Maybe? Really the only time it's relevant is when creating a new labels layer?

GenevieveBuckley commented 10 months ago

Where do you want the warning to go?

I may have misunderstood your comment here, it made it sound like tensorstore was important for performant painting into zarr arrays. Since zarpaint is y'know, a library to enable painting into zarr arrays, I can't think of many situations where someone would open zarpaint but not want to do that.

jni commented 10 months ago

LOL Sorry @GenevieveBuckley, sometimes my mind wanders. 😂 The issue is pretty unambiguous you are 100% correct 😂 but I also have a bit of warning fatigue in general and import-time is a bit yucky cos there's no (quick) way to suppress the warning. I think the right spot is either here:

https://github.com/jni/zarpaint/blob/bcda47846a051077d656c605d780649ec554b4e1/src/zarpaint/_zarpaint.py#L104-L105

or here:

https://github.com/jni/zarpaint/blob/bcda47846a051077d656c605d780649ec554b4e1/src/zarpaint/_zarpaint.py#L12-L13

In the former, you could add a kwarg like warn=True that would suppress the warning if used.

The latter will have a similar effect to what you've implemented, but it's a bit better because it won't add ts to the zarpaint.- namespace. If we end up adding lazy imports, it also won't run until you try to create a tensorstore volume.

Yes zarpaint's primary goal was to enable painting into zarr, but it's also got a bunch of ancillary things such as 3D splitting and (soon one hopes...) slice interpolation. Since zarpaint was created ome-zarr has progressed a fair bit and you can now open zarr files with proper scaling and so forth using a standard file format, so it's less likely that people need to make a zarr volume directly. We do provide a reader but I don't want to maintain a specific ome-zarr reader long-term: probably we should try to contribute/encourage conditional ts support upstream to ome-zarr plugins.

Anyway, if you put the warning in either of the two spots above I'll merge. 😅 Sorry for the confusion! It stems from genuine confusion in my own head due to an ever-evolving ecosystem. 😅

jni commented 10 months ago

Superseded by #49. Thanks for the prod @GenevieveBuckley! 🙏