Hardening PyDataset, Callbacks, and Preprocessing adapt handling across Keras 3 - #23440
Conversation
Fixes issue where invalid class_weight keys were silently ignored in model.fit(), causing sample_weight to default to 1.0 for all samples. Now raises a ValueError with a clear message when a key cannot be converted to an integer class index. Fixes keras-team#23220
Fixes issue where invalid class_weight keys were silently ignored in model.fit(), causing sample_weight to default to 1.0 for all samples. Now raises a ValueError with a clear message when a key cannot be converted to an integer class index. Contributed by Nagabhushana — Fixes keras-team#23220 (Patch Complete)
Hopefully this finally resolves the 'files were modified by this hook' error in the api-gen CI step. The api_gen.py file has now been fully formatted so the hook should see a perfectly clean working tree after it runs. If there are more hidden validation layers, one can only wonder how many errors are hiding like it. Bro, work already!
…et-callbacks-preprocessing-fix
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces relative path traversal warnings in ModelCheckpoint, extracts inputs from tuple/list batches during preprocessing layer adaptation, and improves error handling for PIL's DecompressionBombError in image_utils.py. The review highlights critical issues: the path traversal check is bypassed because os.path.normpath resolves relative segments before the check; _extract_batch incorrectly discards elements when batches are plain 1D lists; and DecompressionBombError is not caught because Image.open() is lazy and needs an explicit img.load() call within the try block.
| normalized_path = os.path.normpath(file_path) | ||
| if ".." in normalized_path.split(os.sep): | ||
| warnings.warn( | ||
| f"ModelCheckpoint filepath '{file_path}' contains relative path " | ||
| "traversal ('..'). Ensure checkpoint directory paths are safe.", | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
The path traversal check is performed on normalized_path after calling os.path.normpath(file_path). Since os.path.normpath resolves relative path segments like .., any traversal sequence (e.g., /safe/dir/../../etc/passwd) will be fully resolved to its target path (e.g., /etc/passwd) before the check is executed. As a result, ".." will not be present in the split segments of the normalized path, completely bypassing the safeguard.\n\nTo fix this, the check should be performed on the original file_path before normalization to ensure any relative path traversal attempts are correctly detected.
if ".." in file_path.replace("/", os.sep).split(os.sep):
warnings.warn(
f"ModelCheckpoint filepath '{file_path}' contains relative path "
"traversal ('..'). Ensure checkpoint directory paths are safe.",
stacklevel=2,
)| def _extract_batch(batch): | ||
| """Return input from batch; handle (x, y) or (x, y, sample_weight).""" | ||
| if isinstance(batch, (tuple, list)): | ||
| return batch[0] | ||
| return batch |
There was a problem hiding this comment.
The current implementation of _extract_batch always returns batch[0] if the batch is a list or tuple. While this correctly extracts the input x from (x, y) or (x, y, sample_weight) tuples, it is completely broken when a custom PyDataset yields a plain 1D list of inputs (e.g., ["apple", "banana"] or [1, 2]). In those cases, _extract_batch will return only the first element (e.g., "apple" or 1), discarding the rest of the batch and leading to incorrect adaptation.\n\nTo fix this, we should check if the first element of the batch is a container (list, tuple, numpy array) or a tensor. If it is, we treat it as a (x, y) structure and return batch[0]. Otherwise, we return the entire batch.
def _extract_batch(batch):
"""Return input from batch; handle (x, y) or (x, y, sample_weight)."""
if isinstance(batch, (tuple, list)) and len(batch) in (2, 3):
if (
isinstance(batch[0], (list, tuple, np.ndarray))
or tf.is_tensor(batch[0])
or backend.is_tensor(batch[0])
):
return batch[0]
return batch| try: | ||
| if isinstance(path, io.BytesIO): | ||
| img = pil_image.open(path) | ||
| elif isinstance(path, (pathlib.Path, bytes, str)): | ||
| if isinstance(path, pathlib.Path): | ||
| path = str(path.resolve()) | ||
| with open(path, "rb") as f: | ||
| img = pil_image.open(io.BytesIO(f.read())) | ||
| else: | ||
| raise TypeError( | ||
| "path must be a string, bytes, pathlib.Path, or BytesIO. " | ||
| f"Received path={path} (of type {type(path)})" | ||
| ) | ||
| except Exception as e: | ||
| if hasattr(pil_image, "DecompressionBombError") and isinstance( | ||
| e, pil_image.DecompressionBombError | ||
| ): | ||
| raise ValueError( | ||
| f"Image load failed for path '{path}' due to PIL DecompressionBombError. " | ||
| "The image exceeds PIL's maximum pixel limit." | ||
| ) from e | ||
| raise e |
There was a problem hiding this comment.
Pillow's Image.open() is lazy and only reads the image headers without decompressing the pixel data. The actual decompression (which triggers DecompressionBombError for oversized images) is deferred until the image is processed or converted (e.g., via img.convert() or img.resize() later in the function).\n\nBecause those processing steps occur outside of this try...except block, any DecompressionBombError will not be caught here and will propagate as an unhandled raw PIL error, bypassing the intended safeguard.\n\nTo fix this, we should explicitly call img.load() inside the try block to force immediate decompression and ensure any decompression bomb errors are caught and handled correctly.
try:
if isinstance(path, io.BytesIO):
img = pil_image.open(path)
elif isinstance(path, (pathlib.Path, bytes, str)):
if isinstance(path, pathlib.Path):
path = str(path.resolve())
with open(path, "rb") as f:
img = pil_image.open(io.BytesIO(f.read()))
else:
raise TypeError(
"path must be a string, bytes, pathlib.Path, or BytesIO. "
f"Received path={path} (of type {type(path)})"
)
img.load()
except Exception as e:
if hasattr(pil_image, "DecompressionBombError") and isinstance(
e, pil_image.DecompressionBombError
):
raise ValueError(
f"Image load failed for path '{path}' due to PIL DecompressionBombError. "
"The image exceeds PIL's maximum pixel limit."
) from e
raise e…processing adapt handling
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #23440 +/- ##
==========================================
- Coverage 84.90% 84.15% -0.75%
==========================================
Files 468 468
Lines 70724 70732 +8
Branches 11715 11718 +3
==========================================
- Hits 60047 59527 -520
- Misses 7667 8204 +537
+ Partials 3010 3001 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Description
Summary
This PR standardizes dataset adaptation handling and hardens callbacks and image utilities across Keras 3:
PyDatasetAdaptation inIndexLookup&TextVectorization:IndexLookup.adapt()andTextVectorization.adapt()to extract feature data from(x, y)and(x, y, sample_weight)tuple/list batches yielded by customPyDatasetdata generators.PyDataset.__init__Parameter Forwarding:**kwargssupport toPyDataset.__init__()signature to allow custom subclasses to safely pass initializers tosuper().__init__().ModelCheckpointPath Safety:..) safeguard warning when saving model checkpoints to custom output file paths.load_imgOversized Image Protection:PIL.Image.DecompressionBombErrorinload_img(), converting raw PIL errors into clear, actionableValueErrorexceptions.Verification & Tests
index_lookup_test.py,text_vectorization_test.py,py_dataset_adapter_test.py,model_checkpoint_test.py, andimage_utils_test.py.Contributor Agreement
Please review our AI-Assisted Contribution Policy and check all boxes below before submitting your PR for review:
~ > Homosapien Here :)