feat: replace mypy and pytype with pyrefly for static type analysis #8868
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Member
There was a problem hiding this comment. The files in this .plans directory shouldn't be included in the PR I don't think. If these were added by another tool we can put |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # PR Review Comments (Copy/Paste Ready) | ||
|
|
||
| ## Comment 1 | ||
| - **File:** `tests/data/test_persistentdataset.py` | ||
| - **Line:** `216` | ||
| - **Severity:** Minor | ||
| - **Comment:** Leftover commented-out line from refactoring. The old `os.path.join` approach is replaced by `Path` usage on line 217, so this comment is dead code. | ||
| - **Suggested change:** Remove line 216. | ||
|
|
||
| ## Comment 2 | ||
| - **File:** `monai/data/dataset.py` | ||
| - **Line:** `387` | ||
| - **Severity:** Minor | ||
| - **Comment:** The local variable is named `data_item_md5` but the hash function now uses sha256 (changed in `utils.py`). This name is misleading — a future reader might assume md5 is still in use. Same issue on line 1621 for `CacheNTransDataset`. | ||
| - **Suggested change:** Rename `data_item_md5` to `data_item_hash` in both locations (lines 387-389 and 1621-1623). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # PR Review Report — #8940 | ||
|
|
||
| ## 1) PR Summary | ||
| Allows `PersistentDataset` to cache `MetaTensor` objects with `weights_only=True` by leveraging MONAI's existing `torch.serialization.add_safe_globals([MetaTensor, ...])` registration. Switches cache-key hashing from md5 to sha256. Addresses GHSA-636w-j999-g7x5. | ||
|
|
||
| ## 2) Template Compliance | ||
| - [x] Description matches implemented changes | ||
| - [x] Linked issue(s) are relevant — security advisory linked | ||
| - [x] Checklist claims match actual changes — new tests added, docstrings updated | ||
| - [ ] Type of change label is accurate — marked "Non-breaking" but see notes below | ||
|
|
||
| ### Notes | ||
| - Marked "Non-breaking change" but the hash-algorithm change (md5 → sha256) invalidates all existing cache files. Old cache files won't be found (hash mismatch), forcing full recomputation and leaving orphaned .pt files on disk. Functionally the API is non-breaking, but users with large pre-built caches will experience an unexpected performance regression. Consider calling this out in a migration note or release changelog. | ||
|
|
||
| ## 3) Findings by Severity | ||
|
|
||
| ### Critical | ||
| - None. | ||
|
|
||
| ### Major | ||
| - **Title:** Hash algorithm change invalidates all existing persistent caches | ||
| - **Severity:** Major | ||
| - **Evidence:** `monai/data/utils.py:1366-1383` (both `json_hashing` and `pickle_hashing`) | ||
| - **Why it matters:** Cache filenames are derived from hash output. After upgrading, none of the existing cache files will match the new sha256 hashes. All data will be recomputed, and old .pt files become orphaned on disk. For users with large cached datasets, this is a significant performance regression and disk-waste concern with no warning. | ||
| - **Suggested fix:** Document prominently in the PR description / changelog. Consider adding a one-time migration helper or a deprecation cycle (accept both hash formats for one release). At minimum, warn users to manually clear their cache directories after upgrading. | ||
|
|
||
| ### Minor | ||
| - **Title:** Dead code comment in new test | ||
| - **Severity:** Minor | ||
| - **Evidence:** `tests/data/test_persistentdataset.py:216` | ||
| - **Why it matters:** The commented-out line `# cache_dir = os.path.join(os.path.join(tempdir, "cache"), "data")` is leftover from refactoring to `Path`. It's noise. | ||
| - **Suggested fix:** Remove the commented-out line. | ||
|
|
||
| - **Title:** Misleading variable name `data_item_md5` persists | ||
| - **Severity:** Minor | ||
| - **Evidence:** `monai/data/dataset.py:387-389` | ||
| - **Why it matters:** After the sha256 switch, the variable name `data_item_md5` is misleading. While pre-existing to this PR, the algorithm change makes this name actively confusing for future readers. | ||
| - **Suggested fix:** Rename to `data_item_hash` or similar. Same for the duplicate at line 1621. | ||
|
|
||
| ## 4) Testing Assessment | ||
| - **Existing tests:** `test_track_meta_and_weights_only` updated — TEST_CASE_5 now expects `MetaTensor` instead of `ValueError`. Covers the new valid combination. | ||
| - **Missing tests:** No test for cache-key collision across hash algorithm change (would require a migration scenario). No test verifying old md5-named cache files are handled gracefully (they're silently ignored — is that the intended behavior?). | ||
| - **Confidence:** Medium — core logic (MetaTensor + weights_only) is well tested. Cache migration behavior is untested. | ||
|
|
||
| ## 5) Needs Author Clarification (if any) | ||
| - Was the silent invalidation of all existing cache files intentional, or should there be a fallback/compatibility path? The PR description calls this "non-breaking" but the behavioral impact on cached datasets is material. | ||
|
|
||
| ## 6) Verdict | ||
| **Verdict:** Approve with comments | ||
|
|
||
| The core change is sound: removing the artificial `track_meta=True` + `weights_only=True` restriction is correct since MetaTensor is registered as a safe global. Tests are thorough and cover both happy path and unsafe-rejection scenarios. The hash algorithm switch to sha256 is a security improvement. Main concern is the undocumented cache-invalidation impact — this should be communicated to users in release notes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # PR Review Comments (Copy/Paste Ready) | ||
|
|
||
| ## Comment 1 | ||
| - **File:** `monai/metrics/meandice.py` | ||
| - **Line:** `437` | ||
| - **Severity:** Major | ||
| - **Comment:** The `and not self.per_component` condition was removed from the `first_ch` assignment. In `per_component=True` + `include_background=True` mode, the old code correctly skipped channel 0 (background has no connected components to analyze). The new code includes it, which changes Dice scores for per_component users. | ||
| - **Suggested change:** Restore the condition: `first_ch = 0 if self.include_background and not self.per_component else 1` | ||
|
|
||
| ## Comment 2 | ||
| - **File:** `monai/metrics/__init__.py` | ||
| - **Line:** `45` | ||
| - **Severity:** Major | ||
| - **Comment:** `create_ignore_mask` is added to `__all__` in `monai/metrics/utils.py` but not imported here. Since losses modules import it from `monai.metrics.utils`, it should be publicly available. External code that needs the same masking logic has no supported import path. | ||
| - **Suggested change:** Add `create_ignore_mask` to the import from `.utils` on this line. | ||
|
|
||
| ## Comment 3 | ||
| - **File:** `monai/losses/utils.py` | ||
| - **Line:** `76` | ||
| - **Severity:** Minor | ||
| - **Comment:** The docstring `"""Apply ignore_index masking to loss inputs."""` is too minimal for a utility function used by four loss classes. Please document the parameters, return type, and contract (e.g., what happens when mask and ignore_index are both None vs one set). | ||
| - **Suggested change:** Add Google-style Args/Returns docstring. | ||
|
|
||
| ## Comment 4 | ||
| - **File:** `tests/losses/test_ignore_index_losses.py` | ||
| - **Line:** `101` | ||
| - **Severity:** Minor | ||
| - **Comment:** `to_onehot_y=True` is passed explicitly here, but some test case kwargs in `SENTINEL_ONEHOT_TEST_CASES` already include it. This could cause a `TypeError` if a loss class rejects duplicate keyword arguments in the future. | ||
| - **Suggested change:** Remove the explicit `to_onehot_y=True` and rely on the kwargs dict, or ensure kwargs never carry `to_onehot_y`. | ||
|
|
||
| ## Comment 5 | ||
| - **File:** `monai/losses/unified_focal_loss.py` | ||
| - **Line:** `250` | ||
| - **Severity:** Minor | ||
| - **Comment:** The shape validation logic in `AsymmetricUnifiedFocalLoss.forward` grew from ~5 lines to ~30+ lines of conditionals. This is now significantly harder to audit for correctness. Consider extracting the validation into a private `_validate_and_prepare_inputs` helper to keep `forward` focused on the loss computation. | ||
| - **Suggested change:** Extract shape validation to a private method; add focused tests for the new branches (binary-to-2-channel, sentinel ignore_index before one_hot, mismatch scenarios). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # PR Review Report — `feat-ignore-index-support` | ||
|
|
||
| ## 1) PR Summary | ||
| Adds an `ignore_index` parameter to segmentation losses (DiceLoss, FocalLoss, TverskyLoss, AsymmetricUnifiedFocalLoss) and metrics (DiceMetric, MeanIoU, GeneralizedDiceScore, HausdorffDistanceMetric, SurfaceDiceMetric, SurfaceDistanceMetric, ConfusionMatrixMetric). A centralized `create_ignore_mask` helper in `monai/metrics/utils.py` generates spatial masks for label-encoded and one-hot targets, while `mask_loss_inputs` in `monai/losses/utils.py` applies them. | ||
|
|
||
| **Note:** No PR has been opened — this review is against branch `feat-ignore-index-support` vs `upstream/dev`. | ||
|
|
||
| ## 2) Template Compliance | ||
| - [x] No PR opened yet — template compliance N/A (review is pre-submission) | ||
|
|
||
| ### Notes | ||
| - No PR body to verify claims against. | ||
|
|
||
| ## 3) Findings by Severity | ||
|
|
||
| ### Critical | ||
| - None. | ||
|
|
||
| ### Major | ||
|
|
||
| - **Title:** `DiceHelper.__call__` `first_ch` logic change alters per_component behavior | ||
| - **Severity:** Major | ||
| - **Evidence:** `monai/metrics/meandice.py:437` — line changed from `first_ch = 0 if self.include_background and not self.per_component else 1` to `first_ch = 0 if self.include_background else 1`. | ||
| - **Why it matters:** In `per_component=True` + `include_background=True` mode, the old code intentionally skipped channel 0 (background) and only computed Dice on the foreground channel. The new code includes channel 0, which is semantically wrong for per_component mode (the background channel has no connected components to analyze) and changes output values for existing users. | ||
| - **Suggested fix:** Restore the `and not self.per_component` condition: `first_ch = 0 if self.include_background and not self.per_component else 1`. | ||
|
|
||
| - **Title:** Cross-package internal import: losses importing from `monai.metrics.utils` | ||
| - **Severity:** Major | ||
| - **Evidence:** Four loss files import `create_ignore_mask` from `monai.metrics.utils`: `monai/losses/dice.py:26`, `monai/losses/focal_loss.py:22`, `monai/losses/tversky.py:21`, `monai/losses/unified_focal_loss.py:20`. Additionally, `monai/losses/utils.py:17` imports from the same metrics module. | ||
| - **Why it matters:** Losses and metrics are sibling packages. Having losses depend on metrics internals at the module level creates a soft import cycle (metrics already import from losses for `LossMetric`). This is architecturally fragile and against the principle that utility layers should not depend on their peers. | ||
| - **Suggested fix:** Move `create_ignore_mask` to a shared utility module (e.g., `monai/utils/` or a new `monai/losses/utils.py` copy) or accept the cycle but document it clearly. | ||
|
|
||
| - **Title:** `create_ignore_mask` not publicly exported from `monai.metrics` package | ||
| - **Severity:** Major | ||
| - **Evidence:** `monai/metrics/utils.py:49` adds `"create_ignore_mask"` to `__all__`, but `monai/metrics/__init__.py` does not import it (line 45 imports other utils but omits `create_ignore_mask`). The loss modules import it via the internal path `monai.metrics.utils.create_ignore_mask`, bypassing the package's public API. | ||
| - **Why it matters:** Users cannot `from monai.metrics import create_ignore_mask`. The function is effectively private yet used as a cross-package dependency. This inconsistency means external code that needs the same masking logic has no supported import path. | ||
| - **Suggested fix:** Add `create_ignore_mask` to the `monai/metrics/__init__.py` imports (line 45), or move it to a shared location and export from there. | ||
|
|
||
| ### Minor | ||
|
|
||
| - **Title:** `mask_loss_inputs` docstring is too minimal | ||
| - **Severity:** Minor | ||
| - **Evidence:** `monai/losses/utils.py:76` — the docstring is a single line: `"""Apply ignore_index masking to loss inputs."""` | ||
| - **Why it matters:** This is a public utility function used by multiple loss classes. It should document its parameters, return type, and contract. | ||
| - **Suggested fix:** Add full Args/Returns docstring following Google style used throughout the codebase. | ||
|
|
||
| - **Title:** Duplicate `to_onehot_y=True` in test parameterization | ||
| - **Severity:** Minor | ||
| - **Evidence:** `tests/losses/test_ignore_index_losses.py:98` — `SENTINEL_ONEHOT_TEST_CASES` already includes `kwargs` that may contain `to_onehot_y`, but line 101 adds `to_onehot_y=True` again. | ||
| - **Why it matters:** This could cause subtle test failures if a future loss class rejects duplicate keyword arguments. Currently benign for `dict.update()` but fragile. | ||
| - **Suggested fix:** Remove the redundant explicit `to_onehot_y=True` from line 101 and rely on kwargs. | ||
|
|
||
| - **Title:** `AsymmetricUnifiedFocalLoss.forward` shape validation rewrite is complex | ||
| - **Severity:** Minor | ||
| - **Evidence:** `monai/losses/unified_focal_loss.py:250-286` — the shape validation logic has been extensively rewritten, adding ~30 lines of conditional checks for `to_onehot_y`, `ignore_index`, binary-to-2-channel conversion, and sentinel value handling. | ||
| - **Why it matters:** The original code had a simple `torch.max(y_true) != self.num_classes - 1` check. The new logic has many branching paths that are harder to reason about. A regression in the shape validation path could silently pass incorrect inputs. | ||
| - **Suggested fix:** Consider extracting the shape validation into a private helper method. Add test cases specifically for the new shape validation branches. | ||
|
|
||
| - **Title:** `get_surface_distance` type narrowing on seg_pred introduces dtype coupling | ||
| - **Severity:** Minor | ||
| - **Evidence:** `monai/metrics/utils.py:345-349` — new `if isinstance(seg_pred, torch.Tensor)` / `else` branch replaces a single generic `dis[seg_pred]` call. | ||
| - **Why it matters:** The old code relied on duck-typing (indexing worked for both torch and numpy). The new code bakes in a torch/numpy split. For cupy inputs this may break since they're not handled by the else branch. | ||
| - **Suggested fix:** Test with cupy inputs or add a cupy branch using `cupy.asarray(seg_pred).astype(bool)`. | ||
|
|
||
| ## 4) Testing Assessment | ||
| - **Existing tests:** Two new test files (`test_ignore_index_losses.py`, `test_ignore_index_metrics.py`) cover ignore_index consistency, no-ignore behavior, class-index masking, and sentinel one-hot masking. Good coverage of the ignore_index feature paths. | ||
| - **Missing tests:** No tests for: | ||
| - `DiceHelper` per_component mode with `ignore_index` | ||
| - Shape validation edge cases in `AsymmetricUnifiedFocalLoss` | ||
| - `get_edge_surface_distance` with `mask` and `warn_empty` parameters | ||
| - `use_subvoxels=True` path with the new areas handling in `get_edge_surface_distance` | ||
| - `compute_hausdorff_distance` with `ignore_index` matching a class index (NaN path) | ||
| - **Confidence:** Medium — the ignore_index core path is well tested, but the per_component regression and `get_edge_surface_distance` changes lack coverage. | ||
|
|
||
| ## 5) Needs Author Clarification (if any) | ||
| - Was the removal of `and not self.per_component` from the `first_ch` assignment in `DiceHelper.__call__` intentional? If not, this is a regression. | ||
| - Is the losses → metrics import direction acceptable to maintainers, or should `create_ignore_mask` be extracted to a shared utility module? | ||
|
|
||
| ## 6) Verdict | ||
| **Verdict:** Request Changes | ||
|
|
||
| The `first_ch` logic change in `DiceHelper.__call__` is likely a regression that silently alters Dice scores in per_component mode. The cross-package import direction and missing public export of `create_ignore_mask` should be resolved before merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ignores should be left in with the pyrefly ones. People will have pytype stuff hanging around still in their own working clones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Restored
.pytype/to.gitignorealongside the pyrefly cache entries.