Adding allow_pickle argument#8875
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNumpyReader gains an allow_pickle constructor parameter (default False) stored on the instance and forwarded to np.load() instead of always using True. Tests assert that loading pickled .npy files or invalid mmap_mode raises ValueError without the flag and succeed when allow_pickle and valid mmap_mode are provided. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
tests/data/test_numpy_reader.py (1)
77-107: ⚡ Quick winAdd one pickled
.npztest for parity.
NumpyReader.read()now gates pickle loading for both.npyand.npz, but the new assertions only exercise.npy. Add one.npzobject-array case for default failure +allow_pickle=Truesuccess.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 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/data/test_numpy_reader.py` around lines 77 - 107, Add a new test mirroring test_npy_pickle but using a pickled .npz archive: create a dict saved with np.savez_compressed (or np.savez) containing an object-array, then assert that NumpyReader.read()/get_data() raises ValueError by default and that constructing NumpyReader(allow_pickle=True) (and calling read with the same mmap kwargs used in test_kwargs) successfully returns the original data; reference the existing test_npy_pickle and test_kwargs patterns and the NumpyReader.read / NumpyReader.get_data calls to implement the default-fail + allow_pickle=True-success assertions for a .npz file.monai/data/image_reader.py (1)
1234-1235: ⚡ Quick winDocument the default failure mode explicitly.
Please add a
Raises:note for the defaultallow_pickle=Falsepath (pickled content load failure), so callers can handle it intentionally.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/data/image_reader.py` around lines 1234 - 1235, Update the docstring for the function that accepts the allow_pickle parameter (the doc in monai/data/image_reader.py) to explicitly document the default failure mode: add a "Raises:" section stating that when allow_pickle=False (the default) loading files that contain pickled/object arrays will raise a ValueError (e.g., "Object arrays cannot be loaded when allow_pickle=False"), so callers can catch/handle it; mention allow_pickle in that Raises entry so the behavior is clearly tied to the parameter.
🤖 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.
Nitpick comments:
In `@monai/data/image_reader.py`:
- Around line 1234-1235: Update the docstring for the function that accepts the
allow_pickle parameter (the doc in monai/data/image_reader.py) to explicitly
document the default failure mode: add a "Raises:" section stating that when
allow_pickle=False (the default) loading files that contain pickled/object
arrays will raise a ValueError (e.g., "Object arrays cannot be loaded when
allow_pickle=False"), so callers can catch/handle it; mention allow_pickle in
that Raises entry so the behavior is clearly tied to the parameter.
In `@tests/data/test_numpy_reader.py`:
- Around line 77-107: Add a new test mirroring test_npy_pickle but using a
pickled .npz archive: create a dict saved with np.savez_compressed (or np.savez)
containing an object-array, then assert that NumpyReader.read()/get_data()
raises ValueError by default and that constructing
NumpyReader(allow_pickle=True) (and calling read with the same mmap kwargs used
in test_kwargs) successfully returns the original data; reference the existing
test_npy_pickle and test_kwargs patterns and the NumpyReader.read /
NumpyReader.get_data calls to implement the default-fail +
allow_pickle=True-success assertions for a .npz file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba124431-25c2-4779-bdb3-a1dbf260e9e2
📒 Files selected for processing (2)
monai/data/image_reader.pytests/data/test_numpy_reader.py
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
1 participant
Addresses GHSA-wg9g-w2j2-8pgr.
Description
This adds
allow_pickle=Falseargument toNumpyReader. This addresses the security concern but allows users to override the setting to load pickled data if needed.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.