quintusdias / glymur

Python interface to OpenJPEG library for reading and writing JPEG 2000 images.
MIT License
61 stars 25 forks source link

Breaking changes in metadata reading v0.12.6 #619

Closed shaneahmed closed 1 year ago

shaneahmed commented 1 year ago

There are some changes in v0.12.6 which is causing deepcopy to fail in TIAToolbox while copying WSI metadata. I couldn't find any changes related to this in the changelog. It would be great if you could help with this.

The PR to fix this can be found here: https://github.com/TissueImageAnalytics/tiatoolbox/pull/612

This is how we are reading the meta data using glymur

https://github.com/TissueImageAnalytics/tiatoolbox/blob/1255ec81dac5fab45baec70be5ae7c92d58ff239/tiatoolbox/wsicore/wsireader.py#L2495

We get the following error at this line https://github.com/TissueImageAnalytics/tiatoolbox/blob/1255ec81dac5fab45baec70be5ae7c92d58ff239/tiatoolbox/wsicore/wsireader.py#L390

x = <_io.BufferedReader name='/home/runner/work/tiatoolbox/tiatoolbox/{envtmpdir}/data3/test1.jp2'>
memo = {[139](https://github.com/TissueImageAnalytics/tiatoolbox/actions/runs/5044292001/jobs/9047147662?pr=612#step:8:140)802281600640: {'axes': 'YXS', 'file_path': PosixPath('/home/runner/work/tiatoolbox/tiatoolbox/{envtmpdir}/data3/te...esolutionBox object at 0x7f2643531ee0>, 139802281637728: glymur.jp2box.CaptureResolutionBox(3636400.0, 3636400.0), ...}
_nil = []

    def deepcopy(x, memo=None, _nil=[]):
        """Deep copy operation on arbitrary Python objects.

        See the module's __doc__ string for more info.
        """

        if memo is None:
            memo = {}

        d = id(x)
        y = memo.get(d, _nil)
        if y is not _nil:
            return y

        cls = type(x)

        copier = _deepcopy_dispatch.get(cls)
        if copier is not None:
            y = copier(x, memo)
        else:
            if issubclass(cls, type):
                y = _deepcopy_atomic(x, memo)
            else:
                copier = getattr(x, "__deepcopy__", None)
                if copier is not None:
                    y = copier(memo)
                else:
                    reductor = dispatch_table.get(cls)
                    if reductor:
                        rv = reductor(x)
                    else:
                        reductor = getattr(x, "__reduce_ex__", None)
                        if reductor is not None:
>                           rv = reductor(4)
E                           TypeError: cannot pickle '_io.BufferedReader' object

/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/copy.py:161: TypeError

Please can you help the information on any change which is causing pickle to fail?

quintusdias commented 1 year ago

Hi Shan,

I'm trying to get an environment thrown together to try to test this, but conda is taking a long, long time trying to install the openslide package. From the stack trace above, it's hard to tell...

In the meantime, my one guess is that the issue is python 3.8 itself. I dropped support for 3.8 back in 0.12.3 since that follows guidance laid out in NEP 29. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it is now heavily used in the test suite. I'll have to review best practices to see if I should have bumped from 0.12.2 all the way to 0.13.0 for dropping 3.8 support even if glymur itself had no api changes.

I'll let you know if/when conda finishes being able to solve an environment for installing openslide.

On Mon, May 22, 2023 at 4:57 AM Shan E Ahmed Raza @.***> wrote:

There are some changes in v0.12.6 which is causing deepcopy to fail in TIAToolbox while copying WSI metadata. I couldn't find any changes related to this in the changelog. It would be great if you could help with this.

The PR to fix this can be found here: TissueImageAnalytics/tiatoolbox#612 https://github.com/TissueImageAnalytics/tiatoolbox/pull/612

This is how we are reading the meta data using glymur

https://github.com/TissueImageAnalytics/tiatoolbox/blob/1255ec81dac5fab45baec70be5ae7c92d58ff239/tiatoolbox/wsicore/wsireader.py#L2495

We get the following error at this line https://github.com/TissueImageAnalytics/tiatoolbox/blob/1255ec81dac5fab45baec70be5ae7c92d58ff239/tiatoolbox/wsicore/wsireader.py#L390

x = <_io.BufferedReader name='/home/runner/work/tiatoolbox/tiatoolbox/{envtmpdir}/data3/test1.jp2'> memo = {139 https://github.com/TissueImageAnalytics/tiatoolbox/actions/runs/5044292001/jobs/9047147662?pr=612#step:8:140802281600640: {'axes': 'YXS', 'file_path': PosixPath('/home/runner/work/tiatoolbox/tiatoolbox/{envtmpdir}/data3/te...esolutionBox object at 0x7f2643531ee0>, 139802281637728: glymur.jp2box.CaptureResolutionBox(3636400.0, 3636400.0), ...} _nil = []

def deepcopy(x, memo=None, _nil=[]): """Deep copy operation on arbitrary Python objects.

See the module's __doc__ string for more info.
"""

if memo is None:
    memo = {}

d = id(x)
y = memo.get(d, _nil)
if y is not _nil:
    return y

cls = type(x)

copier = _deepcopy_dispatch.get(cls)
if copier is not None:
    y = copier(x, memo)
else:
    if issubclass(cls, type):
        y = _deepcopy_atomic(x, memo)
    else:
        copier = getattr(x, "__deepcopy__", None)
        if copier is not None:
            y = copier(memo)
        else:
            reductor = dispatch_table.get(cls)
            if reductor:
                rv = reductor(x)
            else:
                reductor = getattr(x, "__reduce_ex__", None)
                if reductor is not None:

                  rv = reductor(4)

E TypeError: cannot pickle '_io.BufferedReader' object

/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/copy.py:161: TypeError

Please can you help the information on any change which is causing pickle to fail?

— Reply to this email directly, view it on GitHub https://github.com/quintusdias/glymur/issues/619, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVCIXTTLAJR55TU6PLLM73XHNBDJANCNFSM6AAAAAAYKIN5WE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- John Evans

shaneahmed commented 1 year ago

Thanks John. If you are having trouble to install using conda you can try apt-get (on Ubuntu). We have added instructions for various OS here https://tia-toolbox.readthedocs.io/en/latest/installation.html

On Colab, we install using the following https://github.com/TissueImageAnalytics/tiatoolbox/blob/develop/examples/01-wsi-reading.ipynb

!apt-get -y install libopenjp2-7-dev libopenjp2-tools openslide-tools libpixman-1-dev | tail -n 1
!pip install git+https://github.com/TissueImageAnalytics/tiatoolbox.git@develop | tail -n 1

print("Installation is done.")
shaneahmed commented 1 year ago
  1. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it i

I am not clear about support for Python 3.8. If you dropped support for Python 3.8 in 0.12.3 then why pip installing 0.12.6post1 in Python 3.8 environment?

I will trigger GitHub actions for Python 3.9 to check if it fails.

shaneahmed commented 1 year ago
  1. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it i

I am not clear about support for Python 3.8. If you dropped support for Python 3.8 in 0.12.3 then why pip installing 0.12.6 in Python 3.8 environment?

I will trigger GitHub actions for Python 3.9 to check if it fails.

I just checked logs for Python 3.11, it also failed at the same point.

It failed on Python 3.9 as well https://github.com/TissueImageAnalytics/tiatoolbox/actions/runs/5044292001/jobs/9055576599

quintusdias commented 1 year ago

Yeah, I can verify that python 3.8 is NOT the issue.

On Mon, May 22, 2023 at 10:05 AM Shan E Ahmed Raza @.***> wrote:

  1. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it i

I am not clear about support for Python 3.8. If you dropped support for Python 3.8 in 0.12.3 then why pip installing 0.12.6 in Python 3.8 environment?

I will trigger GitHub actions for Python 3.9 to check if it fails.

I just checked logs for Python 3.11, it also failed at the same point.

— Reply to this email directly, view it on GitHub https://github.com/quintusdias/glymur/issues/619#issuecomment-1557497395, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVCIXQOU6HJPNFCKH6KGWDXHOFFBANCNFSM6AAAAAAYKIN5WE . You are receiving this because you commented.Message ID: @.***>

-- John Evans

quintusdias commented 1 year ago

Creating an environment based on your tiatoolbox/requirements.conda.yml file with glymur=0.12.6post1 and the test fails. Downgrading to glymur=0.12.5 and the test succeeds. However, that downgrade pulled in several other changes:

added / updated specs:

The following packages will be downloaded:

package                    |            build
---------------------------|-----------------
glymur-0.12.5              |     pyhd8ed1ab_0         2.6 MB

conda-forge numpy-1.24.3 | py311h64a7726_0 7.5 MB conda-forge openslide-3.4.1 | ha0c88f3_8 109 KB conda-forge

                                       Total:        10.3 MB

The following NEW packages will be INSTALLED:

glymur conda-forge/noarch::glymur-0.12.5-pyhd8ed1ab_0 libblas conda-forge/linux-64::libblas-3.9.0-16_linux64_openblas libcblas conda-forge/linux-64::libcblas-3.9.0-16_linux64_openblas libgfortran-ng conda-forge/linux-64::libgfortran-ng-12.2.0-h69a702a_19 libgfortran5 conda-forge/linux-64::libgfortran5-12.2.0-h337968e_19 liblapack conda-forge/linux-64::liblapack-3.9.0-16_linux64_openblas libopenblas conda-forge/linux-64::libopenblas-0.3.21-pthreads_h78a6416_3 libxslt conda-forge/linux-64::libxslt-1.1.37-h873f0b0_0 lxml conda-forge/linux-64::lxml-4.9.2-py311h14a6109_0 numpy conda-forge/linux-64::numpy-1.24.3-py311h64a7726_0 packaging conda-forge/noarch::packaging-23.1-pyhd8ed1ab_0

The following packages will be DOWNGRADED:

libxml2 2.11.4-h0d562d8_0 --> 2.10.4-hfdac1af_0 openslide 3.4.1-ha896ae7_9 --> 3.4.1-ha0c88f3_8

That's a bit puzzling to me, I'm not sure why downgrading to 0.12.5 would trigger so many changes. Glymur's requirements certainly did not change in 0.12.5.

However, upgrading back to glymur=0.12.6post1 does not revert those changes, only pulling in glymur itself. And the test fails again. So, this is puzzling.

On Mon, May 22, 2023 at 11:55 AM John Evans @.***> wrote:

Yeah, I can verify that python 3.8 is NOT the issue.

On Mon, May 22, 2023 at 10:05 AM Shan E Ahmed Raza < @.***> wrote:

  1. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it i

I am not clear about support for Python 3.8. If you dropped support for Python 3.8 in 0.12.3 then why pip installing 0.12.6 in Python 3.8 environment?

I will trigger GitHub actions for Python 3.9 to check if it fails.

I just checked logs for Python 3.11, it also failed at the same point.

— Reply to this email directly, view it on GitHub https://github.com/quintusdias/glymur/issues/619#issuecomment-1557497395, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVCIXQOU6HJPNFCKH6KGWDXHOFFBANCNFSM6AAAAAAYKIN5WE . You are receiving this because you commented.Message ID: @.***>

-- John Evans

-- John Evans

quintusdias commented 1 year ago

I have an idea what's wrong. I need to come up with a failing test in my own test suite, but then I'll get back to you. Might be a day or so.

On Mon, May 22, 2023 at 12:40 PM John Evans @.***> wrote:

Creating an environment based on your tiatoolbox/requirements.conda.yml file with glymur=0.12.6post1 and the test fails. Downgrading to glymur=0.12.5 and the test succeeds. However, that downgrade pulled in several other changes:

added / updated specs:

  • glymur=0.12.5

The following packages will be downloaded:

package                    |            build
---------------------------|-----------------
glymur-0.12.5              |     pyhd8ed1ab_0         2.6 MB

conda-forge numpy-1.24.3 | py311h64a7726_0 7.5 MB conda-forge openslide-3.4.1 | ha0c88f3_8 109 KB conda-forge

                                       Total:        10.3 MB

The following NEW packages will be INSTALLED:

glymur conda-forge/noarch::glymur-0.12.5-pyhd8ed1ab_0 libblas conda-forge/linux-64::libblas-3.9.0-16_linux64_openblas libcblas conda-forge/linux-64::libcblas-3.9.0-16_linux64_openblas libgfortran-ng conda-forge/linux-64::libgfortran-ng-12.2.0-h69a702a_19 libgfortran5 conda-forge/linux-64::libgfortran5-12.2.0-h337968e_19 liblapack conda-forge/linux-64::liblapack-3.9.0-16_linux64_openblas libopenblas conda-forge/linux-64::libopenblas-0.3.21-pthreads_h78a6416_3 libxslt conda-forge/linux-64::libxslt-1.1.37-h873f0b0_0 lxml conda-forge/linux-64::lxml-4.9.2-py311h14a6109_0 numpy conda-forge/linux-64::numpy-1.24.3-py311h64a7726_0 packaging conda-forge/noarch::packaging-23.1-pyhd8ed1ab_0

The following packages will be DOWNGRADED:

libxml2 2.11.4-h0d562d8_0 --> 2.10.4-hfdac1af_0 openslide 3.4.1-ha896ae7_9 --> 3.4.1-ha0c88f3_8

That's a bit puzzling to me, I'm not sure why downgrading to 0.12.5 would trigger so many changes. Glymur's requirements certainly did not change in 0.12.5.

However, upgrading back to glymur=0.12.6post1 does not revert those changes, only pulling in glymur itself. And the test fails again. So, this is puzzling.

On Mon, May 22, 2023 at 11:55 AM John Evans @.***> wrote:

Yeah, I can verify that python 3.8 is NOT the issue.

On Mon, May 22, 2023 at 10:05 AM Shan E Ahmed Raza < @.***> wrote:

  1. The one python 3.9+ feature being used is importlib.resources.files, which is used in just one place in glymur production code, but it i

I am not clear about support for Python 3.8. If you dropped support for Python 3.8 in 0.12.3 then why pip installing 0.12.6 in Python 3.8 environment?

I will trigger GitHub actions for Python 3.9 to check if it fails.

I just checked logs for Python 3.11, it also failed at the same point.

— Reply to this email directly, view it on GitHub https://github.com/quintusdias/glymur/issues/619#issuecomment-1557497395, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVCIXQOU6HJPNFCKH6KGWDXHOFFBANCNFSM6AAAAAAYKIN5WE . You are receiving this because you commented.Message ID: @.***>

-- John Evans

-- John Evans

-- John Evans

quintusdias commented 1 year ago

Think I got it. Your test passes with my changes. I'll get started releasing v0.12.7, hopefully you can test with it tomorrow morning.

shaneahmed commented 1 year ago

Great, thank you for your prompt response and fixing the issue.

The tests are passing now.