Add Add MSR-VTT benchmark#2235
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| print(f"R@1 : {metrics['recall_at_1']:.4f}") | ||
| print(f"R@5 : {metrics['recall_at_5']:.4f}") | ||
| print(f"R@{top_k:<4d} : {metrics[f'recall_at_{top_k}']:.4f}") |
There was a problem hiding this comment.
--top-k 5 is used. print_summary unconditionally accesses metrics['recall_at_5'] on line 347 and then metrics[f'recall_at_{top_k}'] on line 348 — when top_k == 5 both resolve to the same dict key, so R@5 is printed twice and the R@{top_k} line never shows the actual top-K result. The fix is to only print the hardcoded R@5 row when it is distinct from the dynamic row.
| print(f"R@1 : {metrics['recall_at_1']:.4f}") | |
| print(f"R@5 : {metrics['recall_at_5']:.4f}") | |
| print(f"R@{top_k:<4d} : {metrics[f'recall_at_{top_k}']:.4f}") | |
| print(f"R@1 : {metrics['recall_at_1']:.4f}") | |
| if top_k != _RECALL_AT_5_K: | |
| print(f"R@5 : {metrics['recall_at_5']:.4f}") | |
| print(f"R@{top_k:<4d} : {metrics[f'recall_at_{top_k}']:.4f}") |
| def embed_texts(model: CosmosEmbed1, texts: list[str], batch_size: int) -> np.ndarray: | ||
| """Embed a list of strings; returns L2-normalised (N, D) float32 array.""" | ||
| embs: list[np.ndarray] = [] | ||
| n = len(texts) | ||
| for i, text in enumerate(texts): | ||
| if i % batch_size == 0: | ||
| LOG.info("Text embed %d / %d", i + 1, n) | ||
| t = model.get_text_embedding(text) # (1, D) float16 cpu tensor | ||
| embs.append(t.float().numpy()[0]) | ||
| return _normalise(np.stack(embs, axis=0)) |
There was a problem hiding this comment.
batch_size parameter controls only logging frequency, not actual batching. embed_texts calls model.get_text_embedding(text) once per string regardless of batch_size, so the CLI flag --text-batch-size is misleading to users who expect it to affect throughput or memory. The same pattern appears in embed_videos (line 213: j % max(batch_size, 1) == 0). Consider renaming both parameters to log_interval (or documenting the limitation) so users aren't surprised when changing --text-batch-size has no effect on performance.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _build_video_index(video_dir: Path) -> dict[str, str]: | ||
| """Map {video_id_stem: absolute mp4 path} for everything under video_dir.""" | ||
| index: dict[str, str] = {} | ||
| for p in video_dir.rglob("*.mp4"): | ||
| index.setdefault(p.stem, str(p)) | ||
| return index |
There was a problem hiding this comment.
_build_video_index. If two .mp4 files in different subdirectories share the same filename stem (e.g., train/video0.mp4 and test/video0.mp4), setdefault keeps the first one found and silently drops the second. rglob traversal order is OS-dependent, so the "winning" path is non-deterministic. A debug-level warning when a duplicate stem is encountered would make this easier to diagnose if the dataset zip structure ever changes.
1 participant
Description
Add MSR-VTT benchmark
Usage
python eval/video/bench_msrvtt_retrieval.py \ --model-dir /path/to/models \ --variant 336p \ --output results.jsonChecklist