Expose embed_frames on VideoDataset ingest APIs.#1576
Conversation
📝 WalkthroughWalkthroughVideo dataset ingestion adds an ChangesVideo embedding controls
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
b8b01fa to
6852dff
Compare
Add embed_frames parameter to VideoDataset.add_videos_from_path and add_videos_from_youtube_vis to generate image embeddings for extracted video frames during ingest, independent of video-level embed.
dc92fed to
a2f0190
Compare
…ngs-for-video-frames-python-api-3
|
/review |
| @@ -114,6 +114,110 @@ def test_dataset_add_videos_from_path__dont_embed( | |||
| ) | |||
| assert len(embeddings) == 0 | |||
There was a problem hiding this comment.
This is not touched by you, however still wondering how it fits together
# Check that embeddings were created
[...]
assert len(embeddings) == 0| ) | ||
| assert len(embeddings) == 0 | ||
|
|
||
| def test_dataset_add_videos_from_path__embed_frames( |
There was a problem hiding this comment.
| def test_dataset_add_videos_from_path__embed_frames( | |
| def test_dataset_add_videos_from_path__embed_only_frames( |
MalteEbner
left a comment
There was a problem hiding this comment.
Mostly LGTM
Optional: I am wondering about the 4 tests for the cases
- neither embed frames nor videos
- embed only frames
- embed only videos
- embed frames and videos
I am fine with the current setup, however it seems quite verbose to me. Would be open to refactoring it such that there are less LOC. It would improve readability in my eyes. Could be by parametrisation and/or by putting common code into helper functions.
| ) | ||
| assert len(video_embeddings) == 0 | ||
|
|
||
| def test_dataset_add_videos_from_path__embed_frames_and_video( |
There was a problem hiding this comment.
| def test_dataset_add_videos_from_path__embed_frames_and_video( | |
| def test_dataset_add_videos_from_path__embed_frames_and_videos( |
|
|
||
| ### Added | ||
|
|
||
| - Python SDK: `embed_frames` parameter on `VideoDataset.add_videos_from_path` and `VideoDataset.add_videos_from_youtube_vis` |
There was a problem hiding this comment.
| - Python SDK: `embed_frames` parameter on `VideoDataset.add_videos_from_path` and `VideoDataset.add_videos_from_youtube_vis` | |
| - Python SDK: Allow embedding video frames by adding the `embed_frames` parameter to `VideoDataset.add_videos_from_path` and `VideoDataset.add_videos_from_youtube_vis` |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the changelog prose around the VideoDataset entries to
capitalize the platform name as “YouTube,” while preserving the API identifier
`add_videos_from_youtube_vis` unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e60cfc32-208c-443d-8b34-ec62950ce64f
📒 Files selected for processing (2)
CHANGELOG.mdlightly_studio/tests/core/video/test_video_dataset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lightly_studio/tests/core/video/test_video_dataset.py
What has changed and why?
Add
embed_framestoadd_videos_from_pathandadd_videos_from_youtube_visto generate frame level embeddings, independent of video-level embed.How has it been tested?
New tests
Did you update CHANGELOG.md?
Summary by CodeRabbit
Summary
New Features
embed_framessupport toadd_videos_from_pathandadd_videos_from_youtube_vis, enabling image embeddings for extracted frames during ingest (default: enabled).embed).Documentation
embed_framesbehavior.Tests
embedandembed_frames, including frame-only, video-only, and “no embeddings” scenarios.embed_frameswhere applicable.