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

Warn at import time if tensorstore is missing #47

Closed

Conversation

GenevieveBuckley
Copy link
Contributor

Closes #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
Copy link
Owner

jni commented Oct 18, 2023

@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
Copy link
Contributor Author

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
Copy link
Owner

jni commented Oct 19, 2023

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:

else:
data = labels_temp

or here:

except ModuleNotFoundError:
tensorstore_available = False

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
Copy link
Owner

jni commented Nov 4, 2023

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

@jni jni closed this Nov 4, 2023
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.

Add warning at import time if tensorstore is missing
2 participants