What happens
In ovis/train/dataset/conversation_dataset.py, __getitem__ reads each record's media with read_image / read_video. Both helpers catch I/O and decode errors internally and soft fail to None, returning (None, last_e). The code then asserts the result is not None:
for image_path in image_paths:
image, last_e = self.read_image(image_path)
assert image is not None, f"Failed to read image from {image_path}"
images.append(image)
and the analogous branch for video:
video, last_e = self.read_video(sample, min_frames=self.min_frames, max_frames=self.max_frames)
video_path = sample.get('video') or sample.get('video_frames')
assert video is not None, f"Failed to read video from {video_path}"
Because the readers return None on any unreadable file rather than raising, a single missing or corrupt media reference turns into an AssertionError raised inside a DataLoader worker. The stock HuggingFace Trainer re-raises that in the main process, so the whole training job stops. Every valid record in the same batch, and every later batch, is never processed.
Why this matters
Before this change the dataset logged a warning and skipped the bad record so training could continue. The current code replaced that log-and-continue behavior with the bare asserts, so one unreadable file now halts the run instead of being skipped. The captured last_e is no longer used for anything.
Failure scenario
- A long, multi-GPU SFT run reads a dataset where one record points to a missing or truncated image.
read_image returns None, and assert image is not None fires in a worker.
- The
Trainer aborts the entire run partway through. The operator has to locate and clean the offending record, then restart from the last checkpoint.
Suggested fix
Restore graceful handling: on a None result, log the offending path (the already-captured last_e gives the underlying cause) and skip the record or substitute a placeholder, rather than asserting. That keeps a single bad file from taking down the whole training run.
Reference on the default branch:
|
if 'image' in sample: |
|
images = [] |
|
image_paths = sample['image'] |
|
if isinstance(image_paths, str): |
|
image_paths = [image_paths] |
|
for image_path in image_paths: |
|
image, last_e = self.read_image(image_path) |
|
assert image is not None, f"Failed to read image from {image_path}" |
|
images.append(image) |
|
n_image_or_frame = len(images) |
|
elif 'video' in sample or 'video_frames' in sample: |
|
video, last_e = self.read_video(sample, min_frames=self.min_frames, max_frames=self.max_frames) |
|
video_path = sample.get('video') or sample.get('video_frames') |
|
assert video is not None, f"Failed to read video from {video_path}" |
|
videos = [video] |
Automated report: this issue was produced and filed automatically, with no human review before posting. Two independent checks agreed it is a real bug, but if it misreads the code please say so and we will close it.
Found while running Ito (AI code review that runs your application, free for open source) against recently merged PRs. Full analysis.
What happens
In
ovis/train/dataset/conversation_dataset.py,__getitem__reads each record's media withread_image/read_video. Both helpers catch I/O and decode errors internally and soft fail toNone, returning(None, last_e). The code then asserts the result is notNone:and the analogous branch for video:
Because the readers return
Noneon any unreadable file rather than raising, a single missing or corrupt media reference turns into anAssertionErrorraised inside a DataLoader worker. The stock HuggingFaceTrainerre-raises that in the main process, so the whole training job stops. Every valid record in the same batch, and every later batch, is never processed.Why this matters
Before this change the dataset logged a warning and skipped the bad record so training could continue. The current code replaced that log-and-continue behavior with the bare asserts, so one unreadable file now halts the run instead of being skipped. The captured
last_eis no longer used for anything.Failure scenario
read_imagereturnsNone, andassert image is not Nonefires in a worker.Traineraborts the entire run partway through. The operator has to locate and clean the offending record, then restart from the last checkpoint.Suggested fix
Restore graceful handling: on a
Noneresult, log the offending path (the already-capturedlast_egives the underlying cause) and skip the record or substitute a placeholder, rather than asserting. That keeps a single bad file from taking down the whole training run.Reference on the default branch:
Ovis/ovis/train/dataset/conversation_dataset.py
Lines 32 to 46 in e1916bf
Automated report: this issue was produced and filed automatically, with no human review before posting. Two independent checks agreed it is a real bug, but if it misreads the code please say so and we will close it.
Found while running Ito (AI code review that runs your application, free for open source) against recently merged PRs. Full analysis.