Embed video frames during extraction when embed_frames is enabled.#1575
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds optional embedding generation for extracted video frames. It extends embedding generators and the manager to handle in-memory PIL images, then wires ChangesFrame embedding feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant load_into_collection_from_paths
participant _create_video_frame_samples
participant _flush_frame_batch
participant EmbeddingManager
Caller->>load_into_collection_from_paths: embed_frames=True
load_into_collection_from_paths->>EmbeddingManager: resolve default frame embedding model
load_into_collection_from_paths->>_create_video_frame_samples: FrameExtractionContext(embed_frames, embedding_model_id)
loop per decoded batch
_create_video_frame_samples->>_create_video_frame_samples: collect RGB PIL frames
_create_video_frame_samples->>_flush_frame_batch: flush(frame_sample_ids, pil_frames)
_flush_frame_batch->>EmbeddingManager: embed_and_store_pil_images(...)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightly_studio/src/lightly_studio/core/video/add_videos.py (1)
227-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle embedding failures inside the per-video boundary.
FileOutcomeReport.trackonly suppresses the typed file-outcome signals;_flush_frame_batchcan still raiseRuntimeErroror embedding-manager exceptions, which will escapeload_into_collection_from_pathsand stop the rest of the videos in the batch. Catch and log those here if best-effort ingest is intended.🤖 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 `@lightly_studio/src/lightly_studio/core/video/add_videos.py` at line 227, The per-video exception handling in load_into_collection_from_paths currently only covers file/FFmpeg errors, so failures from _flush_frame_batch or embedding-related RuntimeError paths can still abort the whole batch. Broaden the try/except around the per-video processing block in add_videos.py to catch and log those embedding/flush exceptions at the video boundary, using the existing _flush_frame_batch and FileOutcomeReport.track flow so one bad video does not stop later videos from being processed.
🤖 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.
Outside diff comments:
In `@lightly_studio/src/lightly_studio/core/video/add_videos.py`:
- Line 227: The per-video exception handling in load_into_collection_from_paths
currently only covers file/FFmpeg errors, so failures from _flush_frame_batch or
embedding-related RuntimeError paths can still abort the whole batch. Broaden
the try/except around the per-video processing block in add_videos.py to catch
and log those embedding/flush exceptions at the video boundary, using the
existing _flush_frame_batch and FileOutcomeReport.track flow so one bad video
does not stop later videos from being processed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 066166dd-f002-46cc-aaac-35c8a9aed2d7
📒 Files selected for processing (7)
lightly_studio/src/lightly_studio/core/video/add_videos.pylightly_studio/src/lightly_studio/dataset/embedding_generator.pylightly_studio/src/lightly_studio/dataset/embedding_manager.pylightly_studio/src/lightly_studio/dataset/mobileclip_embedding_generator.pylightly_studio/src/lightly_studio/dataset/perception_encoder_embedding_generator.pylightly_studio/tests/core/video/test_add_videos.pylightly_studio/tests/dataset/test_embedding_manager.py
|
/review |
horatiualmasan
force-pushed
the
horatiu-lig-9970-support-calculating-embeddings-for-video-frames-python-api-2
branch
from
b8b01fa to
6852dff
Compare
horatiualmasan
changed the base branch from
main
to
horatiu-lig-10136-embeddings-for-video-frames-merge-prs
…ngs-for-video-frames-python-api-2
| """ | ||
| ... | ||
|
|
||
| def embed_pil_images( |
There was a problem hiding this comment.
i think it is quite convoluted now. Especially if you imagine users that need to implement this interface. we can go ahead for now, but should consider how to improve this. I.e. i would expect only one embed_images, not two.
There was a problem hiding this comment.
maybe we can address this after we refactor to move the embedding at index time, we could pass directly the in memory image and won't need the variant with file paths
There was a problem hiding this comment.
sgtm make sure to create an issue
There was a problem hiding this comment.
should we also test mobileclip and perception encoder?
…ngs-for-video-frames-python-api-2
…ngs-for-video-frames-python-api-2
2 participants
What has changed and why?
Add embedding manager support for VIDEO_FRAME collections and wire frame embedding into the video ingest path via add_videos.
How has it been tested?
New tests
Did you update CHANGELOG.md?
Summary by CodeRabbit