spyder-ide / spyder-kernels

Jupyter Kernels for the Spyder console
MIT License
39 stars 40 forks source link

PR: Update `load_dicom` to accommodate Pydicom 3.0 #503

Closed mrclary closed 1 month ago

mrclary commented 1 month ago

pydicom released version 3.0 three weeks ago, resulting in recent spyder-kernels test failures for spyder_kernels/utils/tests/test_iofuncs.py::test_load_dicom_files.

Unfortunately, this includes breaking changes. In particular, pydicom.dicomio.read_file is renamed pydicom.dicomio.dcmread. Also unfortunate, pydicom does not support Python <3.10. Also unfortunate, this requirement is neither enforced by pip nor conda for some reason, i.e. pydicom 3.0.1 is installed into Python <3.10 environments.

mrclary commented 1 month ago

@ccordoba12, I think that the environment cache will need to be cleared so that the Linux pip test for Python 3.10 will pass. pydicom 3.0.1 is still being installed instead of 2.4.4.

ccordoba12 commented 1 month ago

Unfortunately, this includes breaking changes. In particular, pydicom.dicomio.read_file is renamed pydicom.dicomio.dcmread.

Ok, then, please change this

try:
    data = dicomio.read_file(filename, force=True)
except TypeError:
    data = dicomio.read_file(filename)

to be

try:
   # For Pydicom 3/Python 3.10+
    data = dicomio.dcmread(filename, force=True)
except TypeError:
    data = dicomio.dcmread(filename)
except AttributeError:
    # For Pydicom 2/Python 3.9-
    try:
        data = dicomio.read_file(filename, force=True)
    except TypeError:
        data = dicomio.read_file(filename)

That will make that code run for Pydicom greater than and less than 3.0.

Also unfortunate, pydicom does not support Python <3.10. Also unfortunate, this requirement is neither enforced by pip nor conda for some reason, i.e. pydicom 3.0.1 is installed into Python <3.10 environments.

Ok, then please skip the test that checks our Pydicom integration for Python versions less than 3.10. And then you can remove the constraint you added to Pydicom.

It'd be nice too if you could inform Pydicom and Conda-forge maintainers that it's 3.0 version only supports 3.10+. But that's up to you.

ccordoba12 commented 1 month ago

Also unfortunate, this requirement is neither enforced by pip nor conda for some reason, i.e. pydicom 3.0.1 is installed into Python <3.10 environments.

The requirement is correctly enforced by pip. Otherwise our tests with pip packages for Python 3.8/3.9 would fail with the last change you did in this PR. So, the problem is only present in Conda-forge.

And in that case, I think the best we can do is to submit a PR to the Pydicom feedstock to fix the situation. If we do that, this shouldn't be necessary:

Ok, then please skip the test that checks our Pydicom integration for Python versions less than 3.10. And then you can remove the constraint you added to Pydicom.

mrclary commented 1 month ago
try:
   # For Pydicom 3/Python 3.10+
    data = dicomio.dcmread(filename, force=True)
except TypeError:
    data = dicomio.dcmread(filename)
except AttributeError:
    # For Pydicom 2/Python 3.9-
    try:
        data = dicomio.read_file(filename, force=True)
    except TypeError:
        data = dicomio.read_file(filename)

That will make that code run for Pydicom greater than and less than 3.0.

Also unfortunate, pydicom does not support Python <3.10. Also unfortunate, this requirement is neither enforced by pip nor conda for some reason, i.e. pydicom 3.0.1 is installed into Python <3.10 environments.

Ok, then please skip the test that checks our Pydicom integration for Python versions less than 3.10. And then you can remove the constraint you added to Pydicom.

If we skip Pydicom integration test for Python <3.10, then there is no need catch the AttributeError.

ccordoba12 commented 1 month ago

If we skip Pydicom integration test for Python <3.10, then there is no need catch the AttributeError

This is not only a matter of fixing our tests. It also has to do with maintaining Pydicom support for Python 3.8/3.9 and 3.10+ at the same time. We need to do that because 3.9 will reach end-of-life in a year or so (3.8 will become unsupported at the end of October though).

mrclary commented 1 month ago

If we skip Pydicom integration test for Python <3.10, then there is no need catch the AttributeError

This is not only a matter of fixing our tests. It also has to do with maintaining Pydicom support for Python 3.8/3.9 and 3.10+ at the same time. We need to do that because 3.9 will reach end-of-life in a year or so (3.8 will become unsupported at the end of October though).

Yep, you are correct. I misspoke.

mrclary commented 1 month ago

@ccordoba12, this is ready for review

mrclary commented 1 month ago

Actually, pydicom = 3.0.1_1 was just built per conda-forge/pydicom-feedstock#40. This should correctly enforce <3.0 for Python <3.10, so we may not have to skip the test.

ccordoba12 commented 1 month ago

@meeseeksdev please backport to 3.x