Add HashMismatchError for download hash validation failures#8861
Add HashMismatchError for download hash validation failures#8861AlexanderSanin wants to merge 5 commits into
Conversation
) Catching BaseException inadvertently suppresses KeyboardInterrupt, SystemExit, and GeneratorExit, which should nearly always propagate. All 17 occurrences across monai/ and tests/ are replaced with Exception, which is the appropriate base class for catchable errors. Signed-off-by: Oleksandr Sanin <alexaaander.sanin@gmail.com>
…MONAI#8832) Introduce a dedicated HashMismatchError (subclass of RuntimeError) to distinguish hash validation failures from network errors in download_url. This allows skip_if_downloading_fails to let hash mismatches propagate as real test failures instead of silently skipping them. - Add HashMismatchError class in monai/apps/utils.py - Use it in download_url when check_hash fails - Update skip_if_downloading_fails to re-raise HashMismatchError - Remove "md5 check" from DOWNLOAD_FAIL_MSGS (no longer needed) - Update test to expect HashMismatchError for bad hash values Signed-off-by: Oleksandr Sanin <alexaaander.sanin@gmail.com>
AlexanderSanin
requested review from
KumoLiu,
Nic-Ma,
ericspod and
virginiafdez
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR tightens exception handling by replacing multiple Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_utils.py (1)
196-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale exception type in test docstring.
The docstring says hash mismatch raises
RuntimeError, but the test now validatesHashMismatchError.Proposed patch
@@ - Raises: - RuntimeError: When the downloaded file's hash does not match. + Raises: + HashMismatchError: When the downloaded file's hash does not match.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils.py` around lines 196 - 201, The test_download_url docstring currently states that a hash mismatch raises RuntimeError but the test actually asserts HashMismatchError; update the Raises section in the test_download_url docstring to list HashMismatchError (the exact exception class name) and optionally add a brief description, ensuring consistency with the test's assertions that raise HashMismatchError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/apps/utils.py`:
- Around line 283-286: The docstring for download_url incorrectly lists
RuntimeError for checksum failures; update the "Raises" section to state
HashMismatchError (the actual exception raised in the implementation) and
include a short description (e.g., when the downloaded file's checksum does not
match expected hash) so the docstring matches the behavior of download_url and
references HashMismatchError.
---
Outside diff comments:
In `@tests/test_utils.py`:
- Around line 196-201: The test_download_url docstring currently states that a
hash mismatch raises RuntimeError but the test actually asserts
HashMismatchError; update the Raises section in the test_download_url docstring
to list HashMismatchError (the exact exception class name) and optionally add a
brief description, ensuring consistency with the test's assertions that raise
HashMismatchError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc3bdf66-94b7-4379-8371-8e06a7a92356
📒 Files selected for processing (15)
monai/__init__.pymonai/apps/__init__.pymonai/apps/auto3dseg/data_analyzer.pymonai/apps/auto3dseg/ensemble_builder.pymonai/apps/auto3dseg/utils.pymonai/apps/detection/metrics/coco.pymonai/apps/nnunet/nnunetv2_runner.pymonai/apps/utils.pymonai/config/deviceconfig.pymonai/data/__init__.pymonai/inferers/inferer.pymonai/utils/tf32.pytests/apps/detection/networks/test_retinanet.pytests/networks/nets/test_resnet.pytests/test_utils.py
| raise HashMismatchError( | ||
| f"{hash_type} check of downloaded file failed: URL={url}, " | ||
| f"filepath={filepath}, expected {hash_type}={hash_val}." | ||
| ) |
There was a problem hiding this comment.
Update download_url Raises docs to HashMismatchError for hash mismatch.
The implementation now raises HashMismatchError, but the docstring still declares RuntimeError for this path.
Proposed patch
@@
- RuntimeError: When the hash validation of the ``url`` downloaded file fails.
+ HashMismatchError: When the hash validation of the ``url`` downloaded file fails.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@monai/apps/utils.py` around lines 283 - 286, The docstring for download_url
incorrectly lists RuntimeError for checksum failures; update the "Raises"
section to state HashMismatchError (the actual exception raised in the
implementation) and include a short description (e.g., when the downloaded
file's checksum does not match expected hash) so the docstring matches the
behavior of download_url and references HashMismatchError.
…@gmail.com> I, Oleksandr Yizchak Sanin <alexaaander.sanin@gmail.com>, hereby add my Signed-off-by to this commit: 5f9ad75 Signed-off-by: Oleksandr Yizchak Sanin <alexaaander.sanin@gmail.com>
Signed-off-by: Oleksandr Yizchak Sanin <alexaaander.sanin@gmail.com>
2 participants
Summary
Fixes #8832
As suggested by @ericspod in #8699, the hash check failure in
download_urlshould raise a specific exception rather than a genericRuntimeError, so it isn't accidentally suppressed byskip_if_downloading_fails.HashMismatchError(RuntimeError)inmonai/apps/utils.pydownload_urlnow raisesHashMismatchErrorwhencheck_hashfailsskip_if_downloading_failsre-raisesHashMismatchErrorimmediately (hash mismatch = real failure, not network issue)"md5 check"fromDOWNLOAD_FAIL_MSGSsince hash failures are now a distinct exception typeHashMismatchErrorinherits fromRuntimeError, so existingexcept RuntimeErrorhandlers still workTest plan
TestDownloadUrltest updated to expectHashMismatchErrorfor bad hash valuesskip_if_downloading_failsSigned-off-by: Oleksandr Sanin alexaaander.sanin@gmail.com