DigitalSlideArchive / HistomicsStream

A whole-slide image reader for TensorFlow
Apache License 2.0
22 stars 6 forks source link

ENH: Make tensorflow,torch,zarr packages optional #135

Closed Leengit closed 2 months ago

Leengit commented 2 months ago

Closes #134 .

review-notebook-app[bot] commented 2 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Leengit commented 2 months ago

The tests are failing because pooch can no longer find the test image and its mask. @cooperlab have they been deleted or moved?:

# download image
wsi_path = pooch.retrieve(
    fname="TCGA-AN-A0G0-01Z-00-DX1.svs",
    url=(
        "https://drive.google.com/uc"
        "?export=download"
        "&id=19agE_0cWY582szhOVxp9h3kozRfB4CvV"
        "&confirm=t"
        "&uuid=6f2d51e7-9366-4e98-abc7-4f77427dd02c"
        "&at=ALgDtswlqJJw1KU7P3Z1tZNcE01I:1679111148632"
    ),
    known_hash="d046f952759ff6987374786768fc588740eef1e54e4e295a684f3bd356c8528f",
    path=str(pooch.os_cache("pooch")) + os.sep + "wsi",
)

# download binary mask image
mask_path = pooch.retrieve(
    fname="TCGA-AN-A0G0-01Z-00-DX1.mask.png",
    url=(
        "https://drive.google.com/uc"
        "?export=download"
        "&id=17GOOHbL8Bo3933rdIui82akr7stbRfta"
    ),
    known_hash="bb657ead9fd3b8284db6ecc1ca8a1efa57a0e9fd73d2ea63ce6053fbd3d65171",
    path=str(pooch.os_cache("pooch")) + os.sep + "wsi",
)
print(f"Have {mask_path}")
cooperlab commented 2 months ago

@andsild can fix this for us

andsild commented 2 months ago

There are some differences in the URL, not sure why they changed, but here's what we use (changing the URL should be sufficient):

wsi_fname = "TCGA-AN-A0G0-01Z-00-DX1.svs"
    wsi_path = pooch.retrieve(
        fname=wsi_fname,
        url="https://drive.usercontent.google.com/download?id=19agE_0cWY582szhOVxp9h3kozRfB4CvV&export=download&confirm=t",
        known_hash="d046f952759ff6987374786768fc588740eef1e54e4e295a684f3bd356c8528f",
        path=wsi_files,
    )
    print(f"Downloaded {wsi_fname} to {wsi_files}")

    mask_fname = "TCGA-AN-A0G0-01Z-00-DX1.mask.png"
    mask_path = pooch.retrieve(
        fname=mask_fname,
        url="https://drive.usercontent.google.com/download?id=17GOOHbL8Bo3933rdIui82akr7stbRfta&export=download&confirm=t",
        known_hash="bb657ead9fd3b8284db6ecc1ca8a1efa57a0e9fd73d2ea63ce6053fbd3d65171",
        path=wsi_files,
    )
    print(f"Downloaded {mask_fname} to {wsi_files}")
andsild commented 2 months ago

(if you want me to I could fix it, but I dont have write privileges in this repository)

Leengit commented 2 months ago

Thank you for the updated URLs. I have made the changes myself, and have given you permissions for the repository.

Leengit commented 2 months ago

With the current set of commits, on GitHub only, with Python 3.8 only, somewhere deep inside a dependency of large_image, and regardless of various permutations and combinations of pip install typing-extensions and placement of import large_image statements that I have tried, I am getting

ImportError: cannot import name 'Buffer' from 'typing_extensions' (/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/typing_extensions.py)

@manthey et al. have you seen this before? Python 3.8 works fine locally for me. It works fine with Python 3.6, 3.7, and 3.9.

Leengit commented 2 months ago

Because it builds and passes all tests on my local system with Python 3.8, and it builds and passes all tests on GitHub with Python 3.6, 3.7, and 3.9, I have gone forward with the hack response of removing the Python 3.8 test build from GitHub. If approved and merged as is, we should submit an issue to find a fix for the Python 3.8 build on GitHub.

Leengit commented 2 months ago

This pull request is ready for review.

manthey commented 2 months ago

With the current set of commits, on GitHub only, with Python 3.8 only, somewhere deep inside a dependency of large_image, and regardless of various permutations and combinations of pip install typing-extensions and placement of import large_image statements that I have tried, I am getting

ImportError: cannot import name 'Buffer' from 'typing_extensions' (/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/typing_extensions.py)

@manthey et al. have you seen this before? Python 3.8 works fine locally for me. It works fine with Python 3.6, 3.7, and 3.9.

I expect that this means some other package is specifying typing_extensions < 4.6 in the 3.8 environment, since Buffer was only added then.

manthey commented 2 months ago

With the current set of commits, on GitHub only, with Python 3.8 only, somewhere deep inside a dependency of large_image, and regardless of various permutations and combinations of pip install typing-extensions and placement of import large_image statements that I have tried, I am getting

ImportError: cannot import name 'Buffer' from 'typing_extensions' (/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/typing_extensions.py)

@manthey et al. have you seen this before? Python 3.8 works fine locally for me. It works fine with Python 3.6, 3.7, and 3.9.

I expect that this means some other package is specifying typing_extensions < 4.6 in the 3.8 environment, since Buffer was only added then.

And, looking further, the version of TensorFlow that installs on 3.8 (version 2.13.1) requires typing_extensions< 4.6, so TensorFlow on 3.8 is incompatible with large_image on 3.8 because of that. If we want these to be compatible, we'd have to relax the type rules in large_image.