jni / zarpaint

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

Removing Tensorstore Dependency #41

Closed Lencripe closed 10 months ago

Lencripe commented 1 year ago

Tensorstore not required after the zarr update.

Lencripe commented 1 year ago

@DragaDoncila Please review.

DragaDoncila commented 1 year ago

Also @jni you probs want to review (and approve workflow πŸ˜† )

jni commented 1 year ago

I don't love that napari.yaml got reformatted too... Do you know what tool did that? (It shouldn't be yapf but I don't actually know.)

jni commented 1 year ago

Sorry, wrong button πŸ˜…

The tests are failing because you need to move the tensorstore dependency from install_requires to options.extras_require / testing, rather than deleting it. After that I think this is ready to go!

Lencripe commented 1 year ago

Hi Juan I initially tried to change the syntax of the code using my IDE however I later realised it wasn't using the yapf formatter so that may explain the changes made to the napery.yaml file. I have however reverted those changes by checking out the napari.yaml from an earlier commit.

image

I spent sometime yesterday reading through pre-commit's documentation and I finally managed to fix my weird error πŸ₯³which as it turns out was not related to pre-commit at all πŸ˜‚ however the information I learnt should be useful when I start working on the pre-commit & GitHub actions for Napari-graph.I've also moved tensorstore dependency to testing so hopefully the tests pass this time 🀞🏾.

jni commented 1 year ago

Hmmm, ok, so it looks like there's more changes to be made. Sorry, I am not very familiar with tox, which is used by the cookiecutter template for plugins to set up all of the testing configurations. So right now it only installs the dependencies, not the testing dependencies. That's a problem! πŸ˜… In order to get the extras, you can add an extras = testing to tox.ini, as done in napari here:

https://github.com/napari/napari/blob/c871c319247b79d4c5cc9270c0fec04740aed9d0/tox.ini#L79-L83

Another issue is that the solution isn't quite what we're after: we don't want to have open_tensorstore not use tensorstore. What we want is the following:

try:
    import tensorstore
except ModuleNotFoundError as e:
    msg = 'You must install the optional tensorstore library to open a zarr file with it.'
    raise ModuleNotFoundError(msg) from e

Does this all make sense?

GenevieveBuckley commented 10 months ago

I think this PR is superseded by https://github.com/jni/zarpaint/pull/44. We can probably safely close this now.

Thank you for all your early work pushing this forward @Lencripe, it is much appreciated

jni commented 10 months ago

Indeed, and thanks @GenevieveBuckley for the reminder here. πŸ˜