Skip to content

Add hash_type default deprecation warnings to apps utils [pre-commit.ci autofix] #9124

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.

Sign up for GitHub

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

Closed
Copilot wants to merge 5 commits into from
+198 −5
Closed

Add hash_type default deprecation warnings to apps utils [pre-commit.ci autofix] #9124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d66a4c
Initial plan
Copilot Sep 21, 2026
376249c
Add hash_type default deprecation warnings
Copilot Sep 21, 2026
a79fdf8
Refine hash_type deprecation coverage
Copilot Sep 21, 2026
7e25cd0
Apply suggestion from @ericspod
ericspod Sep 21, 2026
72a9d3b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 58 additions & 2 deletions monai/apps/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from urllib.request import urlopen, urlretrieve

from monai.config.type_definitions import PathLike
from monai.utils import look_up_option, min_version, optional_import
from monai.utils import deprecated_arg_default, look_up_option, min_version, optional_import

requests, has_requests = optional_import("requests")
gdown, has_gdown = optional_import("gdown", "4.7.3")
Expand All @@ -54,6 +54,12 @@

DEFAULT_FMT = "%(asctime)s - %(levelname)s - %(message)s"
SUPPORTED_HASH_TYPES = {"md5": hashlib.md5, "sha1": hashlib.sha1, "sha256": hashlib.sha256, "sha512": hashlib.sha512}
_HASH_TYPE_DEFAULT_CHANGE_VERSION = "1.6.1"

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using deprecated_arg_default here is the right call, but I think this particular "since" value makes the whole mechanism inert on exactly the builds that carry the breaking change.

deprecated_arg_default skips installation entirely when the running version is at or below "since" — see the is_not_yet_deprecated branch in monai/utils/deprecate_utils.py around line 283, which returns an identity decorator so no warning is ever emitted. The dev branch currently versions as 1.6.1rc0+N.g, which orders strictly before 1.6.1, so on every dev build and every 1.6.1 release candidate all four decorators in this file are no-ops.

Reproduced on a checkout of this branch:

monai.__version__ = 1.6.1rc0+6.g72a9d3b9
version_leq(v, "1.6.1") = True  and  v != "1.6.1"  ->  decorator inert
check_hash(filepath, sha256)  with hash_type omitted  ->  0 FutureWarnings

The same cause makes the new test fail locally on a checkout that has tags:

tests/apps/test_check_hash.py:105: AssertionError: 0 != 1
1 failed, 10 passed

CI stays green only because the unit-test jobs check out shallow without tags, so the version resolves to 0+untagged and deprecate_utils substitutes sys.maxsize, which activates the warning. Copying the same tree without .git gives 11 passed. So the green checks on this PR do not demonstrate that the warning works for users.

Since the hardening landed after 1.6.0 (1.6.1rc0 is the first tag containing it), 1.6.0 looks like the correct value: it is already released and strictly below every current build, so the warning fires on 1.6.1rc0, 1.6.1 and later. I confirmed version_leq(v, "1.6.0") is False for the current dev version.

Suggested change:

_HASH_TYPE_DEFAULT_CHANGE_VERSION = "1.6.0"

@ericspod ericspod Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
_HASH_TYPE_DEFAULT_CHANGE_VERSION = "1.6.1"
_HASH_TYPE_DEFAULT_CHANGE_VERSION = "1.6.0"

_HASH_TYPE_DEFAULT_WARNING_REMOVAL_VERSION = "1.8"

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In deprecated_arg_default, "replaced" means the version at which the default was or will be replaced, not the version at which the warning itself is retired. With replaced set to 1.8, the message a user sees on a 1.6.x or 1.7.x build is:

Current default value of argument hash_type="md5" has been deprecated since version 1.6.1. It will be changed to hash_type="sha256" in version 1.8.

But the default already is sha256 today, on line 183 of this same file. Someone who has just hit a HashCheckError reads "it will be changed in version 1.8" and reasonably concludes their breakage must be something else — the opposite of the guidance this PR is trying to give.

Setting replaced to the version where the change actually happened produces the correct wording, which reads "was changed in version from hash_type="md5" to hash_type="sha256"".

If we also want to communicate a horizon for removing the warning itself, that belongs in msg_suffix rather than in replaced. The constant name encodes the same misreading and is worth renaming alongside it.

Suggested change: pass the version in which the default actually changed as replaced=, and move any "warning removed in 1.8" note into the message suffix.

_HASH_TYPE_DEFAULT_CHANGE_MSG = (
"The default was updated to SHA-256 for stronger integrity verification. "
'Pass `hash_type="md5"` explicitly only when you need backward compatibility with existing MD5 hashes.'
)


class HashCheckError(ValueError):
Expand Down Expand Up @@ -166,10 +172,22 @@ def safe_extract_member(member, extract_to):
return full_path


@deprecated_arg_default(
"hash_type",
old_default='"md5"',
new_default='"sha256"',

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

old_default and new_default are being passed as strings that contain literal quote characters. That matters for more than cosmetics: deprecated_arg_default has a guard around line 310 of monai/utils/deprecate_utils.py that raises ValueError when the declared new_default already equals the function's actual default but "replaced" says the change is still in the future. Because the string '"sha256"' never equals "sha256", that guard is silently bypassed.

I checked both forms against this exact signature:

without quotes -> ValueError: Argument hash_type was replaced to the new default value sha256 before the specified version 1.8.
with quotes    -> no ValueError, guard bypassed

That ValueError is the utility correctly diagnosing the inconsistency I raised in the comment about replaced=1.8 above. Once replaced is corrected to the version where the default really changed, the unquoted values pass the guard legitimately and we keep the validation for the future.

The same applies to the three other decorator blocks in this file, at lines 223-224, 347-348 and 437-438.

Suggested change:

old_default="md5",
new_default="sha256",

since=_HASH_TYPE_DEFAULT_CHANGE_VERSION,
replaced=_HASH_TYPE_DEFAULT_WARNING_REMOVAL_VERSION,
msg_suffix=_HASH_TYPE_DEFAULT_CHANGE_MSG,
)
def check_hash(filepath: PathLike, val: str | None = None, hash_type: str = "sha256") -> bool:
"""
Verify hash signature of specified file.

.. versionchanged:: 1.6.1
The default ``hash_type`` changed from ``"md5"`` to ``"sha256"`` for stronger integrity verification.
Pass ``hash_type="md5"`` explicitly only when you need backward compatibility with existing MD5 hashes.

Args:
filepath: path of source file to verify hash value.
val: expected hash value of the file.
Expand Down Expand Up @@ -200,6 +218,14 @@ def check_hash(filepath: PathLike, val: str | None = None, hash_type: str = "sha
return True


@deprecated_arg_default(

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

deprecated_arg_default warns purely on argument absence, before the function body runs. But hash_type only has any effect when a hash value is actually supplied: download_url consults it at lines 273 and 303, and extractall at line 387 behind an "if hash_val and ..." guard. So a call with hash_val=None warns about a hash algorithm that is never used. Confirmed on a build where the decorator is active:

download_url(hash_val=None)  ->  1 FutureWarning

That matters because MONAI itself calls these functions without hash_type and usually without a hash value at all. I found 17 such call sites: monai/bundle/scripts.py lines 199, 200, 206, 207, 216, 217, 263, 568 and 569; monai/networks/nets/hovernet.py 635 and 670; monai/networks/nets/senet.py 304; monai/networks/nets/swin_unetr.py 1319; monai/apps/auto3dseg/bundle_gen.py 431; monai/apps/tcia/utils.py 103; monai/transforms/utils_create_transform_ims.py 215. Once the version gating is fixed, every bundle download and every pretrained-weights fetch will print an MD5-to-SHA-256 migration notice that has nothing to do with what the user asked for. Warning noise at that scale tends to train people to ignore MONAI warnings, which undercuts the goal of this PR.

Two ways out. Either keep the decorator only on check_hash and let the others inherit it through the call chain — worth noting the chain does not currently double-warn, since download_and_extract forwards hash_type explicitly, and I measured exactly one warning for a fully defaulted download_and_extract call — or keep all four decorators and first make the internal call sites pass hash_type explicitly, so MONAI never warns about itself. The second is the smaller behavioural change and documents intent at each call site.

"hash_type",
old_default='"md5"',
new_default='"sha256"',
since=_HASH_TYPE_DEFAULT_CHANGE_VERSION,
replaced=_HASH_TYPE_DEFAULT_WARNING_REMOVAL_VERSION,
msg_suffix=_HASH_TYPE_DEFAULT_CHANGE_MSG,
)
def download_url(
url: str,
filepath: PathLike = "",
Expand All @@ -211,6 +237,10 @@ def download_url(
"""
Download file from specified URL link, support process bar and hash check.

.. versionchanged:: 1.6.1
The default ``hash_type`` changed from ``"md5"`` to ``"sha256"`` for stronger integrity verification.
Pass ``hash_type="md5"`` explicitly only when you need backward compatibility with existing MD5 hashes.

Args:
url: source URL link to download file.
filepath: target filepath to save the downloaded file (including the filename).
Expand Down Expand Up @@ -312,6 +342,14 @@ def _extract_tar(filepath, output_dir):
shutil.copyfileobj(source, target)


@deprecated_arg_default(
"hash_type",
old_default='"md5"',
new_default='"sha256"',
since=_HASH_TYPE_DEFAULT_CHANGE_VERSION,
replaced=_HASH_TYPE_DEFAULT_WARNING_REMOVAL_VERSION,
msg_suffix=_HASH_TYPE_DEFAULT_CHANGE_MSG,
)
def extractall(
filepath: PathLike,
output_dir: PathLike = ".",
Expand All @@ -324,6 +362,10 @@ def extractall(
Extract file to the output directory.
Expected file types are: `zip`, `tar.gz` and `tar`.

.. versionchanged:: 1.6.1
The default ``hash_type`` changed from ``"md5"`` to ``"sha256"`` for stronger integrity verification.
Pass ``hash_type="md5"`` explicitly only when you need backward compatibility with existing MD5 hashes.

Args:
filepath: the file path of compressed file.
output_dir: target directory to save extracted files.
Expand Down Expand Up @@ -390,6 +432,14 @@ def get_filename_from_url(data_url: str) -> str:
raise Exception(f"Error processing URL: {e}") from e


@deprecated_arg_default(
"hash_type",
old_default='"md5"',
new_default='"sha256"',
since=_HASH_TYPE_DEFAULT_CHANGE_VERSION,
replaced=_HASH_TYPE_DEFAULT_WARNING_REMOVAL_VERSION,
msg_suffix=_HASH_TYPE_DEFAULT_CHANGE_MSG,
)
def download_and_extract(
url: str,
filepath: PathLike = "",
Expand All @@ -403,6 +453,10 @@ def download_and_extract(
"""
Download file from URL and extract it to the output directory.

.. versionchanged:: 1.6.1
The default ``hash_type`` changed from ``"md5"`` to ``"sha256"`` for stronger integrity verification.
Pass ``hash_type="md5"`` explicitly only when you need backward compatibility with existing MD5 hashes.

Args:
url: source URL link to download file.
filepath: the file path of the downloaded compressed file.
Expand Down Expand Up @@ -433,4 +487,6 @@ def download_and_extract(
with tempfile.TemporaryDirectory() as tmp_dir:
filename = filepath or Path(tmp_dir, get_filename_from_url(url)).resolve()
download_url(url=url, filepath=filename, hash_val=hash_val, hash_type=hash_type, progress=progress)
extractall(filepath=filename, output_dir=output_dir, file_type=file_type, has_base=has_base)
extractall(
filepath=filename, output_dir=output_dir, hash_type=hash_type, file_type=file_type, has_base=has_base
)
143 changes: 140 additions & 3 deletions tests/apps/test_check_hash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@

import hashlib
import os
import shutil
import tempfile
import unittest
import warnings
import zipfile
from unittest.mock import patch

import numpy as np
from parameterized import parameterized

from monai.apps import check_hash
from monai.apps import check_hash, download_and_extract, download_url, extractall

TEST_CASE_1 = ["b94716452086a054208395e8c9d1ae2a", "md5", True]

Expand All @@ -33,6 +37,31 @@


class TestCheckMD5(unittest.TestCase):
@staticmethod
def _hash_file(filename, hash_type):
hash_func = getattr(hashlib, hash_type)
with open(filename, "rb") as f:
return hash_func(f.read()).hexdigest()

@staticmethod
def _write_file(filename, content=b"monai hash fixture"):
with open(filename, "wb") as f:
f.write(content)

@classmethod
def _download_side_effect(cls, source):
def _copy(_url, destination, progress=True):
del progress
shutil.copyfile(source, destination)

return _copy

@classmethod
def _create_zip_fixture(cls, tempdir):
archive = os.path.join(tempdir, "fixture.zip")
with zipfile.ZipFile(archive, "w") as zip_file:
zip_file.writestr("fixture/data.txt", "monai")
return archive

@parameterized.expand([TEST_CASE_1, TEST_CASE_2, TEST_CASE_3, TEST_CASE_4, TEST_CASE_5])
def test_result(self, md5_value, t, expected_result):
Expand All @@ -55,7 +84,7 @@ def test_warns_when_val_is_none(self):
filename = os.path.join(tempdir, "test_file.png")
test_image.tofile(filename)
with self.assertWarns(UserWarning):
result = check_hash(filename, None)
result = check_hash(filename, None, hash_type="sha256")
self.assertTrue(result)

def test_default_hash_type_is_sha256(self):
Expand All @@ -64,7 +93,115 @@ def test_default_hash_type_is_sha256(self):
filename = os.path.join(tempdir, "test_file.png")
test_image.tofile(filename)
sha256 = hashlib.sha256(test_image.tobytes()).hexdigest()
self.assertTrue(check_hash(filename, sha256))
self.assertTrue(check_hash(filename, sha256, hash_type="sha256"))

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test was introduced by the hardening PR specifically to prove that the default hash type is sha256. Passing hash_type="sha256" explicitly makes the assertion tautological — it would still pass if someone reverted the default to "md5", which is exactly the regression the test exists to catch. Silencing the new warning by removing the coverage it was meant to protect is a net loss.

Better to keep the call implicit and handle the warning around it, for example:

with warnings.catch_warnings():
    warnings.simplefilter("ignore", FutureWarning)
    self.assertTrue(check_hash(filename, sha256))

or by asserting both the returned value and the warning in the same test. The same consideration applies to test_warns_when_val_is_none at line 87.


def test_omitting_hash_type_emits_future_warning(self):
def assert_single_future_warning(callable_obj, *args, **kwargs):
with warnings.catch_warnings(record=True) as recorded:
warnings.simplefilter("always")
callable_obj(*args, **kwargs)

future_warnings = [w for w in recorded if issubclass(w.category, FutureWarning)]
self.assertEqual(len(future_warnings), 1)

@garciadias garciadias Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This assertion is only true when the running MONAI version is past the "since" value, so its outcome depends on whether the checkout has git tags. On a tagged clone of this branch it fails with 0 != 1; on the same tree without .git it passes. Even after the version gating is corrected, it would be worth making that expectation explicit rather than incidental — deprecated_arg_default accepts a version_val parameter precisely so tests can pin it.

Patching the private helper _download_with_progress at lines 119, 125, 151, 165, 190 and 199 also couples these tests to an internal that could move.

Suggested change: construct the decorator under test with an explicit version_val, or add a visible version guard so the assumption is stated rather than inherited from the checkout.

self.assertIn('hash_type="md5"', str(future_warnings[0].message))
self.assertIn('hash_type="sha256"', str(future_warnings[0].message))

with tempfile.TemporaryDirectory() as tempdir:
filename = os.path.join(tempdir, "fixture.bin")
self._write_file(filename)
sha256 = self._hash_file(filename, "sha256")

download_target = os.path.join(tempdir, "downloaded.bin")
archive = self._create_zip_fixture(tempdir)
archive_sha256 = self._hash_file(archive, "sha256")

assert_single_future_warning(check_hash, filename, sha256)
with patch("monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(filename)):
assert_single_future_warning(
download_url, "https://example.com/fixture.bin", download_target, hash_val=sha256, progress=False
)
assert_single_future_warning(extractall, archive, os.path.join(tempdir, "extract"), hash_val=archive_sha256)
with (
patch("monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(archive)),
patch("monai.apps.utils.get_filename_from_url", return_value="fixture.zip"),
):
assert_single_future_warning(
download_and_extract,
"https://example.com/fixture.zip",
filepath=os.path.join(tempdir, "downloaded.zip"),
output_dir=os.path.join(tempdir, "download_and_extract"),
hash_val=archive_sha256,
progress=False,
)

def test_explicit_sha256_does_not_emit_default_change_warning(self):
with tempfile.TemporaryDirectory() as tempdir:
filename = os.path.join(tempdir, "fixture.bin")
self._write_file(filename)
sha256 = self._hash_file(filename, "sha256")

download_target = os.path.join(tempdir, "downloaded.bin")
archive = self._create_zip_fixture(tempdir)
archive_sha256 = self._hash_file(archive, "sha256")

with warnings.catch_warnings(record=True) as recorded:
warnings.simplefilter("always")
self.assertTrue(check_hash(filename, sha256, hash_type="sha256"))
with patch(
"monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(filename)
):
download_url(
"https://example.com/fixture.bin",
download_target,
hash_val=sha256,
hash_type="sha256",
progress=False,
)
extractall(archive, os.path.join(tempdir, "extract"), hash_val=archive_sha256, hash_type="sha256")
with (
patch("monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(archive)),
patch("monai.apps.utils.get_filename_from_url", return_value="fixture.zip"),
):
download_and_extract(
"https://example.com/fixture.zip",
filepath=os.path.join(tempdir, "downloaded.zip"),
output_dir=os.path.join(tempdir, "download_and_extract"),
hash_val=archive_sha256,
hash_type="sha256",
progress=False,
)

future_warnings = [w for w in recorded if issubclass(w.category, FutureWarning)]
self.assertEqual(future_warnings, [])

def test_explicit_md5_still_works(self):
with tempfile.TemporaryDirectory() as tempdir:
filename = os.path.join(tempdir, "fixture.bin")
self._write_file(filename)
md5 = self._hash_file(filename, "md5")

download_target = os.path.join(tempdir, "downloaded.bin")
archive = self._create_zip_fixture(tempdir)
archive_md5 = self._hash_file(archive, "md5")

self.assertTrue(check_hash(filename, md5, hash_type="md5"))
with patch("monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(filename)):
download_url(
"https://example.com/fixture.bin", download_target, hash_val=md5, hash_type="md5", progress=False
)
extractall(archive, os.path.join(tempdir, "extract"), hash_val=archive_md5, hash_type="md5")
with (
patch("monai.apps.utils._download_with_progress", side_effect=self._download_side_effect(archive)),
patch("monai.apps.utils.get_filename_from_url", return_value="fixture.zip"),
):
download_and_extract(
"https://example.com/fixture.zip",
filepath=os.path.join(tempdir, "downloaded.zip"),
output_dir=os.path.join(tempdir, "download_and_extract"),
hash_val=archive_md5,
hash_type="md5",
progress=False,
)


if __name__ == "__main__":
Expand Down
Loading