chore: Dependency Maintenance (imageio 2->3, pyBIDS<0.15.6, readthedocs.yml v2, Python 3.7->3.8)

[kanishk16]

Checklist

GitHub

• I've given this PR a concise, self-descriptive, and meaningful title
• I've linked relevant issues in the PR body
• I've applied the relevant labels to this PR
• I've assigned a reviewer

PR contents

• I've consulted ivadomed's internal developer documentation to ensure my contribution is in line with any relevant design decisions
• I've added relevant tests for my contribution
• I've updated the relevant documentation for my changes, including argparse descriptions, docstrings, and ReadTheDocs tutorial pages

Description

This PR combines a number of maintenance changes, updating various dependencies that have evolved since the last release of ivadomed.

1. imageio 2->3

imageio had planned a pretty user-friendly (slow and steady) transition to v3, imageio/imageio#725, for a year and a half. It is now approaching its stage to bump to v3. We initially made some hotfixes to move to v2, #1181, rather than migrating to v3, considering it to be in its early days - stabilizing & gathering user feedback. But, it appears to have stabilized after a significant number of minor and patch releases, especially in the last 2-3 months (imageio/imageio#982, imageio/imageio#931).

Moreover, the recent imageio releases raised quite a few issues #1291 that were high-priority concerns originating from axondeepseg/axondeepseg#738 and axondeepseg/axondeepseg#737. There were attempts to hotfix with #1292 and #1296, but the tests were still broken. Since imageio.v3 appears much more stable, this PR attempts to streamline and resolve the above issues by migrating to imageio.v3.

Further details about the migration itself can be found in this comment: #1297 (comment)

2. pyBIDS<0.15.6

In recent months, pyBIDS has introduced some backwards incompatible changes that break our tests in multiple ways: #1297 (comment), #1297 (comment).

We could update ivadomed to be compatible with the newest PyBIDS, however in the short term it's simplest just to pin <0.15.6. 0.15.5 was released in November 2022 and its wheel is not limited to specific versions of Python 3, so there should be no forwards-compatibility issues.

3. Pillow <10.0.0

Pillow 10.0.0 is incompatible with the outdated version of tensorboard we use. This fix will be unnecessary once we update torch in #1304.

See: #1297 (comment)

4. readthedocs.yml v2

See: https://blog.readthedocs.com/migrate-configuration-v2/

5. Python 3.7->3.8

Python 3.7 has reached EOL, so it needed to be updated in our tests workflow as well as the readthedocs workflow.

6. Ubuntu 18.04

Reached EOL.

Linked issues

Resolves #1291

[hermancollin]

Thanks @kanishk16 for addressing this. Looks like a bunch of tests are now passing

[kanishk16]

@hermancollin Though most tests pass, I'd like you to test & verify as in #1296. Also, it would be great if you could check the inference too.

@dyt811 Since windows tests aren't passing for some time (unrelated to the current PR) and are required before merging, could you remove the windows tests from the required statuses - Python 3.8 and Ubuntu-18.04, Python 3.8 (#1298) to unblock this PR?

[kanishk16]

Ubuntu-18.04, Python 3.8 (#1298) required check also needs to be removed as GitHub Actions stopped the support for Ubuntu-18.04 LTS quite some time back.

[mathieuboudreau]

@dyt811 Since windows tests aren't passing for some time (unrelated to the current PR) and are required before merging, could you remove the windows tests from the required statuses - Python 3.8 and Ubuntu-18.04, Python 3.8 (#1298) to unblock this PR?

Or we could look into how to resolve the one test that's failing in Windows - has someone looked at it yet / is there an issue/PR? Because all the other tests pass in windows, and it would be a shame to stop testing on windows just for this edge case.

[mathieuboudreau]

If I were to guess, it has to do with this line

which is formatted for unix-based systems only (MacOS/Linux), vs the line above that uses Path() and will correctly format the path to whichever system is being used.

[mathieuboudreau]

I think I resolved the bug - essentially pybids changed how they parse regex over the winter, and at some point it started breaking for Windows. Restricting pybids to <15.6.0 resolved the failing test in the draft PR I setup: #1299, see: https://github.com/ivadomed/ivadomed/actions/runs/5467608625/jobs/9954141022

Let me make the change here, and if they all pass, I'll reintroduce the required windows checks and we can move ahead. @hermancollin were you intending on reviewing this PR too?

[kanishk16]

I think I resolved the bug - essentially pybids changed how they parse regex over the winter, and at some point it started breaking for Windows. Restricting pybids to <15.6.0 resolved the failing test in the draft PR I setup: #1299, see: https://github.com/ivadomed/ivadomed/actions/runs/5467608625/jobs/9954141022

@mathieuboudreau I'm not in favor of restricting pybids for a windows test as:

1. Blocks future developments from bids to be integrated into ADS which might be much more significant
2. "PyBIDS currently supports Python 3 on POSIX operating systems (including Mac OS). Windows is not officially supported, though most PyBIDS functionality will probably work fine." (ref: https://github.com/bids-standard/pybids)

Dependency pinning is much more complex than it seems - for detailed discussions #1191

[mathieuboudreau]

@joshuacwnewton after reviewing this thread, I think it may be ready to merge as-is, but requires an admin to force-merge due to the failing windows tests (unrelated to this PR - #1297 (comment)).

The last comment in this thread was made by me, expressing some concern for (seemingly arbitrary) hardcoded values that were coded to convert RGBA to greyscale, now that we couldn't use mode= as_gray" anymore: #1297 (comment)

However, I dug a little bit into the old imageio and pillow codebases, and it appears that as_gray used these same hardcoded values (though using an int16 integer set) to do this conversion (https://github.com/python-pillow/Pillow/blob/b723e9e62e4706a85f7e44cb42b3d838dae5e546/src/PIL/ImageColor.py#L152), so all is well here and I'm not satisfied with knowing that there appears to be consistency between how RGBA was converted to grayscale before to now.

So, either you can do re-review if you'd like for your own purposes, or you or I can force-merge this PR now!

[FirefoxMetzger]

FYI @mathieuboudreau the "random" numbers used in RGB to grayscale conversion go back to Recommendation BT.601-7, which is a document on standardizing colors for television and was first published in the 80s.

In the linked document you will find said formula in Annex 2 (page 15, equation 1). The annex describes how to convert sRGB into YCrCb, and the equation everybody uses is this conversion's Y (luminance) component. The numbers aren't uniform because the human eye has different sensitivity to each of the primary colors and choosing them this way is "true to human perception".

From there we get RGBA to grayscale by effectively ignoring the alpha component. There is only one image, so no blending is happening, and subsequently alpha doesn't change the result.

[mathieuboudreau]

FYI @mathieuboudreau the "random" numbers used in RGB to grayscale conversion go back to Recommendation BT.601-7, which is a document on standardizing colors for television and was first published in the 80s.

In the linked document you will find said formula in Annex 2 (page 15, equation 1). The annex describes how to convert sRGB into YCrCb, and the equation everybody uses is this conversion's Y (luminance) component. The numbers aren't uniform because the human eye has different sensitivity to each of the primary colors and choosing them this way is "true to human perception".

From there we get RGBA to grayscale by effectively ignoring the alpha component. There is only one image, so no blending is happening, and subsequently alpha doesn't change the result.

Wow, thank you so much for clarifying where these came from @FirefoxMetzger! My background is in physics and not CS, so this is not something I would have encountered previously :)

[kanishk16]

Thank you @FirefoxMetzger, for the insightful discussion, reviews, and helpful suggestions. BTW, this bug helped me explore imageio extensively and v3 simplifies a lot of the internal stuff making it easier to dive into the codebase.

[kanishk16]

I'll try to summarize the essence of the changes.

imageio provides a unified interface to read and write a wide range of image data from many 3 rd party libs.

How does imageio work under the hood to load an image? (https://imageio.readthedocs.io/en/latest/user_guide/overview.html)

imageio core comprises user-facing APIs and a plugin manager. A plugin manager intelligently configures a plugin (an adapter cum wrapper to backend library, a potential 3rd party lib that responds to a request from imageio.core handling installs and imports related cases) either by searching for a specific plugin as per the priority mentioned in the docs or by sending the request to the plugin you specified explicitly.

Now, you send requests to imageio.core through the user-facing API that relays it to the plugin manager, which selects a plugin from a set of plugins to determine which backend to fulfill your request.

Previously, using imageio v2:

if "tif" in extension:
    img = np.expand_dims(imageio.v2.imread(filename, format='tiff-pil'), axis=-1).astype(np.uint8)
    if len(img.shape) > 3:
        img = np.expand_dims(imageio.v2.imread(filename, format='tiff-pil', as_gray=True), axis=-1).astype(np.uint8)
    else:
        img = np.expand_dims(imageio.v2.imread(filename, as_gray=True), axis=-1).astype(np.uint8)

• .tif -- plugin: TIFF-PIL + backend lib: pillow
• .png -- plugin: PNG-PIL + backend lib: pillow
• as_gray=True converts the img to grayscale using ITU-R 601-2 luma transform as discussed above.

The change from v2 to v3 (_img = imageio.v3.imread(filename)) introduced new defaults:

• .tif -- plugin: tifffile + backend lib: tifffile and
• .png -- plugin: pillow + backend lib: pillow.

So, the recent commit migrates from v2 to v3, while preserving the same plugin and backend libs as before with v2:

if '.tif' in extension:
    img = np.expand_dims(imageio.v3.imread(filename, plugin='TIFF-PIL'), axis=-1).astype(np.unint8)      
    if len(img.shape) > 3:
        img = np.expand_dims(imageio.v3.imread(filename, plugin='TIFF-PIL', as_gray=True), axis=-1).astype(np.unint8)
    else:
        img = np.expand_dims(imageio.v3.imread(filename, plugin='PNG-PIL', as_gray=True), axis=-1).astype(np.unint8)

So, this handles all the cases whenever a user wants to have the original behaviour (more or less consistent with what @FirefoxMetzger suggested) as well as the case that @hermancollin encountered #1297 (comment).

@mathieuboudreau Hopefully, this clarifies the commit. Let me know if you have any other suggestions.

[joshuacwnewton]

Hi folks! I'm finally back to help get this PR merged. 🎉

I've updated the title and description of this PR, as well as hidden some conversations that have since been resolved, to help make this PR a little easier to parse.

---

In reviewing the most recent updates to this PR (1424f61, Oct 2, 2023), I noticed new test failures stemming from an entirely different PyBIDS-related bug:

            # Filter dataframe to keep subjects files with available derivatives only
            if has_deriv:
                self.df = self.df[self.df['filename'].str.contains('|'.join(has_deriv))
                                  | self.df['filename'].str.contains('|'.join(deriv))]
            else:
                # Raise error and exit if no derivatives are found for any subject files
>               raise RuntimeError("Derivatives not found.")
E               RuntimeError: Derivatives not found.

This seems to be due to a newer version of PyBIDS, as pinning PyBIDS to an older version fixes this error.

We could spend time debugging this new issue and update ivadomed such that it is compatible with newer PyBIDS versions. But, given that we were already somewhat open to the idea of pinning PyBIDS due to the previous Windows-related bug (as previously discussed in (#1297 (comment), #1297 (comment), etc.), I'd like to propose pinning PyBIDS in the short term.

If we truly need to unpin PyBIDS in the future (e.g. for a dependent project), we can always revisit how we want to update the ivadomed-specific code in the future. But, I think pinning is a good short-term patch, given that ivadomed as a whole is in maintenance mode.

---

I've also added some new changes to the .readthedocs.yml file to ensure it complies with RTD's new config file requirements, and updated the title of this PR accordingly.

[joshuacwnewton]

This has my approval, but it seems wise to have one other ivadomed member review my proposed changes re: Pillow/PyBIDS. :)

[kanishk16]

In reviewing the most recent updates to this PR (1424f61, Oct 2, 2023), I noticed new test failures stemming from an entirely different PyBIDS-related bug:

            # Filter dataframe to keep subjects files with available derivatives only
            if has_deriv:
                self.df = self.df[self.df['filename'].str.contains('|'.join(has_deriv))
                                  | self.df['filename'].str.contains('|'.join(deriv))]
            else:
                # Raise error and exit if no derivatives are found for any subject files
>               raise RuntimeError("Derivatives not found.")
E               RuntimeError: Derivatives not found.

This seems to be due to a newer version of PyBIDS, as pinning PyBIDS to an older version fixes this error.

I'd like to drop it here to document the little debugging done to pursue this bug previously (also raised in #1305 a few weeks back). This bug originates from the pybids 0.16.2 release. Although the debugging wasn't conclusive enough to raise a patch PR, a hacky fix was to set the bids_validate key to false under the loader_parameters in the config.json. In case we need a long-term fix in place, it would be good to figure out what happens differently with and without bids validation in pybids. We should then raise a PR as pybids seems to be less actively maintained.

We could spend time debugging this new issue and update ivadomed such that it is compatible with newer PyBIDS versions. But, given that we were already somewhat open to the idea of pinning PyBIDS due to the previous Windows-related bug (as previously discussed in (#1297 (comment), #1297 (comment), etc.), I'd like to propose pinning PyBIDS in the short term.

If we truly need to unpin PyBIDS in the future (e.g. for a dependent project), we can always revisit how we want to update the ivadomed-specific code in the future. But, I think pinning is a good short-term patch, given that ivadomed as a whole is in maintenance mode.

I was not in favour of pinning pybids then as discussed earlier as at that time we were still transitioning to maintenance mode (primarily with respect to ADS) but given the time since then, I don't have any strong opinion about pinning as a short-term patch. Moreover, afaik ADS has transitioned to monai from training models to benchmarking metrics (surpassing the ivadomed benchmarks).

I've also added some new changes to the .readthedocs.yml file to ensure it complies with RTD's new config file requirements, and updated the title of this PR accordingly.

@joshuacwnewton Thanks for your help. LGTM and feel free to merge (but since I opened the PR I can't officially approve xD).

[joshuacwnewton]

Thank you for adding the additional context! I'm glad we have an issue open for further debugging the PyBIDS issues later on if need be. :)