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

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

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

Leengit
Copy link
Collaborator

@Leengit Leengit commented Sep 16, 2024

Closes #134 .

@Leengit Leengit added the enhancement New feature or request label Sep 16, 2024
@Leengit Leengit self-assigned this Sep 16, 2024
@Leengit Leengit marked this pull request as draft September 16, 2024 19:27
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 16, 2024

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

@andsild can fix this for us

@andsild
Copy link
Collaborator

andsild commented Sep 16, 2024

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

andsild commented Sep 16, 2024

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

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 16, 2024

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

@Leengit Leengit force-pushed the optional_packages branch 3 times, most recently from 060980d to a7517fa Compare September 16, 2024 23:32
@Leengit
Copy link
Collaborator Author

Leengit commented Sep 16, 2024

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 Leengit force-pushed the optional_packages branch 2 times, most recently from f558495 to 79c6d92 Compare September 17, 2024 13:42
@Leengit Leengit marked this pull request as ready for review September 17, 2024 13:49
@Leengit
Copy link
Collaborator Author

Leengit commented Sep 17, 2024

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
Copy link
Collaborator Author

Leengit commented Sep 17, 2024

This pull request is ready for review.

@manthey
Copy link

manthey commented Sep 17, 2024

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

manthey commented Sep 17, 2024

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.

Copy link

@manthey manthey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, though we could further adjust tests for python versions.

Also, run `pip install --upgrade typing-extensions` on GitHub after
installing the tensorflow dependency of histomics_stream because
tensorflow under Python 3.8 inappropriately downgrades
typing-extensions and that breaks a dependency of large_image.
@Leengit Leengit merged commit 19f0f29 into DigitalSlideArchive:master Sep 17, 2024
6 checks passed
@Leengit Leengit deleted the optional_packages branch September 17, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make torch and tensorflow be optional installation dependencies
4 participants