Repository navigation
Conversation
Disable load_from_mmar before any download or model loading, retain the public signature, and deprecate the MMAR APIs for 1.7. Replace skipped network tests with offline regression coverage and update the API docs. Signed-off-by: Muhammad Talha <muhammadtalha223@gmail.com> Assisted-by: OpenAI Codex <noreply@openai.com>
Use the full release version in the MMAR deprecation decorators and test the actual APIs before and after that boundary. Keep loader rejection unconditional and scope the file-access guard to the MMAR module. Signed-off-by: Muhammad Talha <muhammadtalha223@gmail.com> Assisted-by: OpenAI Codex <noreply@openai.com> Assisted-by: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Issue #9162 requires MMAR support to be deprecated and load_from_mmar to be disabled. monai/apps/mmars/mmars.py deprecates the MMAR module and public functions, and load_from_mmar raises `Deprec… |
| Out of Scope Changes check | |
The changes in mmars.py, model_desc.py, docs/source/apps.rst, and tests/apps/test_mmar_download.py implement or document the deprecation and loader disablement required by #9162. The test repl… |
| Docstring Coverage | |
Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: 1 … |
| Title check | |
The title clearly and concisely describes the main changes: deprecating MMAR APIs and disabling load_from_mmar. |
| Description check | |
The description is complete and follows the template. It explains the security change, deprecation schedule, retained APIs, tests, validation results, documentation updates, and intentionally unclaime… |
Full details: Linked Issues check
-
Explanation
Issue
#9162requires MMAR support to be deprecated andload_from_mmarto be disabled.monai/apps/mmars/mmars.pydeprecates the MMAR module and public functions, andload_from_mmarraisesDeprecatedErrorbefore download or loading.get_model_specanddownload_mmarretain lookup and download behavior. The updated tests cover rejection, public exports, deprecation warnings, and no loading side effects.
Full details: Out of Scope Changes check
-
Explanation
The changes in
mmars.py,model_desc.py,docs/source/apps.rst, andtests/apps/test_mmar_download.pyimplement or document the deprecation and loader disablement required by#9162. The test replacement supports these requirements. No unrelated change is demonstrated.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: 1 unsupported.)
Full details: Description check
-
Explanation
The description is complete and follows the template. It explains the security change, deprecation schedule, retained APIs, tests, validation results, documentation updates, and intentionally unclaimed full-suite checks.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
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 help to get the list of available commands.
There was a problem hiding this comment.
Hi @Talhax55z thanks for this, I think it's doing what we need for security though I had minor changes. I would like @garciadias to have a look in relation to the relevant advisory.
| __all__ = ["MODEL_DESC", "RemoteMMARKeys"] | ||
|
|
||
|
|
||
| class RemoteMMARKeys: |
There was a problem hiding this comment.
This class should be marked as deprecated as well, eventually this whole mmar directory will be removed.
| __all__ = ["get_model_spec", "download_mmar", "load_from_mmar"] | ||
|
|
||
|
|
||
| @deprecated(since="1.7.0", msg_suffix="NVIDIA Clara MMAR support is deprecated. Please use MONAI Bundles instead.") |
There was a problem hiding this comment.
Typically things are deprecated for two releases so you can add the argument removed="1.9.0". The other deprecated decorators the same.
Deprecate RemoteMMARKeys and schedule the class and all three MMAR functions for removal in 1.9.0. Keep model loading disabled immediately. Extend release-boundary tests and document the test cases and removal schedule. Signed-off-by: Muhammad Talha <muhammadtalha223@gmail.com> Assisted-by: OpenAI Codex <noreply@openai.com>
| removed="1.9.0", | ||
| msg_suffix="NVIDIA Clara MMAR support is deprecated. Please use MONAI Bundles instead.", | ||
| ) | ||
| def download_mmar( |
There was a problem hiding this comment.
One deprecation ends up being reported twice here. download_mmar is decorated, and for a string item it delegates to get_model_spec, which carries the same decorator, so on 1.7.0 a single call such as download_mmar("clara_pt_prostate_mri_segmentation_1", mmar_dir=".", api=False) emits two FutureWarnings, one of them attributed to get_model_spec rather than to the function the caller invoked. Confirmed at this head commit by re-executing the module with version_val set to 1.7.0 and recording warnings: that call produced two FutureWarnings. Routing the internal lookup through a private undecorated helper would make a public call warn exactly once.
There was a problem hiding this comment.
Do we just suppress the warning for the call to get_model_spec? The alternative is move the body of get_model_spec into an auxiliary undecorated function which it and download_mmar call.
| removed="1.9.0", | ||
| msg_suffix="NVIDIA Clara MMAR support is deprecated. Please use MONAI Bundles instead.", | ||
| ) | ||
| class RemoteMMARKeys: |
There was a problem hiding this comment.
RemoteMMARKeys is a constants namespace, so this decorator only fires on instantiation, which nothing in the code base does. Verified at this head commit with version_val set to 1.7.0: attribute access such as RemoteMMARKeys.NAME emits no warning; with 1.9.0, attribute access still returns the constants while instantiating raises DeprecatedError. MODEL_DESC has the same property. The new test covers the instantiation path, so it does not exercise what users would actually observe. If the intent is simply that this module disappears at 1.9.0 then that is reasonable, but a sentence to that effect in the deprecation note (or a test on the attribute-access path) would make the schedule meaningful.
garciadias
left a comment
garciadias
left a comment
There was a problem hiding this comment.
Reviewed the change against issue #9162 and the security advisory it references.
What I verified at e344df1:
- load_from_mmar raises DeprecatedError before any download, file access or deserialization, and the public imports and signature are unchanged, so callers get a clear error rather than a silent fallback.
- The deprecation schedule requested in review is in place for all four public definitions (get_model_spec, download_mmar, load_from_mmar, RemoteMMARKeys): since 1.7.0, removed 1.9.0. Both earlier comments on this PR are addressed by the head commit.
- The old loading implementation and the now-unused private helpers were removed with no remaining references anywhere in the tree.
- Locally, tests/apps/test_mmar_download.py plus tests/utils/test_deprecated.py give 71 passed with no skips, and ruff and black are clean on the changed files.
The two offline observations I left inline are cosmetic and do not affect the security goal, so I am not blocking on them. The only thing to keep in mind is that the remote test coverage for the still-supported download path is now mock-based, which is a reasonable trade-off for an offline suite.
Thanks for turning this around quickly.
This branch has not been deployed
4 participants
Fixes #9162.
Description
MMAR support is deprecated in favor of MONAI Bundles. For security reasons,
load_from_mmarnow raisesDeprecatedErrorbefore downloading or loading a model. The old loading code is removed, while the public imports and signature remain available to give callers a clear error.The three MMAR functions and
RemoteMMARKeysare deprecated at1.7.0and scheduled for removal in1.9.0. Lookup and download still work until removal. The loader is disabled immediately, including on development versions before1.7.0.The skipped MMAR suite is replaced with 40 offline tests covering loader rejection, public exports, lookup/download behavior, and deprecation warnings and removal behavior across the
1.7.0and1.9.0release boundaries. Class constants and model descriptors remain intact. The loader is tested with warnings ignored and its decorator unwrapped.Validation
On Windows with Python 3.12.7:
The documentation check uses a small Sphinx project with a stub for the existing bundle link. Full-suite, full-documentation, and coverage results are not claimed. CI is rerunning for the review update.
Types of changes
Prepared with Astra 6 assistance and reviewed with Claude and by myself, Also deepseek was also evaluvating the issue . Both are credited in the commit trailers.