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

Pull upstream ivadomed changes that let us upgrade previously-pinned versions of dipy/numpy/nibabel #4332

Merged
merged 19 commits into from Mar 19, 2024

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Jan 10, 2024

Description

Upstream ivadomed changes: ivadomed/ivadomed#1308

The upstream changes enabled a cascade of version upgrades (compared to master):

  • Explicit dependency upgrades (requirements.txt):
    • nibabel: 3.2.2 -> 5.2.1
    • dipy: 1.5.0 -> 1.8.0
    • numpy: 1.23.5 -> 1.26.4
  • Second-order dependency upgrades (held back by previous packages):
    • pandas: 1.4.4 -> 1.5.3
    • pyparsing: 2.4.7 -> 3.1.2

Given that we've been held back on these versions for so long, some additional changes are needed to address upstream deprecation warnings and errors, mainly due to numpy.

Linked issues

Fixes #4319.
Mostly addresses #4408, but doesn't address #4408 (comment).

@joshuacwnewton joshuacwnewton added the installation category: install_sct or pip/setup.py label Jan 10, 2024
@joshuacwnewton joshuacwnewton added this to the 6.2 milestone Jan 10, 2024
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@joshuacwnewton joshuacwnewton modified the milestones: 6.2, 6.3 Feb 12, 2024
@joshuacwnewton joshuacwnewton changed the title Test upstream ivadomed changes to avoid dependency conflicts with dipy Update ivadomed to allow us to upgrade to the newest versions of dipy and numpy Mar 12, 2024
This incorporates the merged changes from ivadomed/ivadomed#1308, which
allow use to upgrade dipy and numpy.
This wasn't possible before, but with the upstream ivadomed changes, we can now allow newer
versions of dipy.
On 64-bit systems, creating a dummy image with `dtype=int` will result in `int64` images. But, `nibabel` doesn't recommend using `int64` due to compatibility issues, so we explicitly specify `int32` when creating dummy images.
In addition to what the "pystrum" comment says, there is an additional reason why we have
been pinning numpy: an incompatibility with our old pin of `dipy==1.5.0`

(See: #4314 (comment))

Now that we've upgraded to `dipy==1.8.0` and above, we can safely unpin `numpy` too.
@joshuacwnewton joshuacwnewton force-pushed the jn/4319-fix-ivadomed-dependency-conflict branch from 3e84a2f to 67b1f6b Compare March 15, 2024 01:49
This commit is to test whether there is a meaningful difference between dipy==1.8.0 and dipy==1.9.0.

It's possible that this has an effect on the stalling test, but it's also possible that the stalling
tests were entirely flukes due to model-downloading timeouts.

If this commit and the previous commit both pass, then `dipy` makes no difference. But if this
commit passes and the previous one fails, then `dipy` does make a difference.
requirements.txt Outdated Show resolved Hide resolved
@joshuacwnewton joshuacwnewton changed the title Update ivadomed to allow us to upgrade to the newest versions of dipy and numpy Pull upstream ivadomed changes that let us upgrade previously-pinned versions of dipy/numpy/nibabel Mar 15, 2024
These files were erroneously triggering CI failures for the "fake" git links.
A new release (ivadomed v2.9.10) has been created containing all of the changes from the master
branch. So, we can just install the newest version from PyPI.
The `sct_deepseg_gm` test uses dummy input, which results in the `ort_sess.run` function
returning `nan` as output. This then causes a `RuntimeWarning: invalid value encountered in cast`
when postprocessing the output. So, to be explicit about these nan values, I'm replacing them with
0 (which would have been done anyway during the casting):

>>> np.array(np.nan).astype(np.uint8)
<input>:1: RuntimeWarning: invalid value encountered in cast
array(0, dtype=uint8)
This is following the direct instructions of the warning message:

DeprecationWarning: `np.math` is a deprecated alias for the standard library `math` module (Deprecated Numpy 1.25). Replace usages of `np.math` with `math`
This test currently doesn't work as intended: #4405

The test itself is of minimal importance, since it tests a nonsense input (-initz -1) that shouldn't
be used in normal processing.
We catch the "in divide" message, but we don't catch the "in scalar divide" warning.

Tracked in #4407.
@joshuacwnewton joshuacwnewton force-pushed the jn/4319-fix-ivadomed-dependency-conflict branch from 1f57611 to 91b0e69 Compare March 18, 2024 21:45
I had mistakenly thought this was resolved (because `wandb` did indeed resolve it on their end),
but this is still not resolved for `monai`. I have reported this upstream: Project-MONAI/MONAI#7559
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

Great work, thanks. It almost feels like spring cleaning! I'll add a commit in a few minutes (see comment below), then it should be good to merge.

spinalcordtoolbox/scripts/sct_analyze_lesion.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
testing/api/test_qmri.py Show resolved Hide resolved
@joshuacwnewton joshuacwnewton merged commit 56c7f6c into master Mar 19, 2024
20 checks passed
@joshuacwnewton joshuacwnewton deleted the jn/4319-fix-ivadomed-dependency-conflict branch March 19, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation category: install_sct or pip/setup.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dipy==1.8.0 causes trx-python dependency conflict with ivadomed, resulting in failing SCT installations
2 participants