[bugfix] fix bug in srt_api.py#826
Conversation
WalkthroughTime-instruction handling in generation was fixed and gated: the variable name was corrected to Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SRT_API
participant VideoDecoder
Caller->>SRT_API: generate(..., modality, imgs, add_time_instruction)
alt modality != "video" or imgs is None or add_time_instruction == false
SRT_API-->>Caller: proceed without time instruction
else modality == "video" and imgs exists and add_time_instruction == true
SRT_API->>VideoDecoder: decode video (get frame_time, video_time)
VideoDecoder-->>SRT_API: frame_time, video_time
SRT_API->>SRT_API: build time_instruction using frame_time/video_time
SRT_API-->>Caller: include time instruction in prompt
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lmms_eval/models/simple/srt_api.py (3)
185-189: Prevent NameError; fix typo and spacing; wrap to 88 cols
time_instrucitonis misspelled, may be undefined (non‑video or decode error), and the sentence lacks a space before “Please”. Also exceeds 88 chars.Apply:
- time_instruciton = f"The video lasts for {video_time:.2f} seconds, and {len(frames)} frames are uniformly sampled from it. These frames are located at {frame_time}.Please answer the following questions related to this video." + time_instruction = ( + f"The video lasts for {video_time:.2f} seconds, and " + f"{len(frames)} frames are uniformly sampled from it. " + f"These frames are located at {frame_time}. Please answer " + f"the following questions related to this video." + ) - if self.add_time_instruction: - contexts = f"{time_instruciton}\n{contexts}" + if ( + self.add_time_instruction + and self.modality == "video" + and "time_instruction" in locals() + ): + contexts = f"{time_instruction}\n{contexts}"Suggested cleaner follow‑up (outside this hunk): initialize
time_instruction: str | None = Nonebefore the loop; set it in the try block; then checkif self.add_time_instruction and time_instruction:to avoidlocals()usage.
246-250: Mirror the async fix in sync pathSame issues as above: typo, potential undefined variable, missing space, long line. Keep both paths consistent.
Apply:
- time_instruciton = f"The video lasts for {video_time:.2f} seconds, and {len(frames)} frames are uniformly sampled from it. These frames are located at {frame_time}.Please answer the following questions related to this video." + time_instruction = ( + f"The video lasts for {video_time:.2f} seconds, and " + f"{len(frames)} frames are uniformly sampled from it. " + f"These frames are located at {frame_time}. Please answer " + f"the following questions related to this video." + ) - if self.add_time_instruction: - contexts = f"{time_instruciton}\n{contexts}" + if ( + self.add_time_instruction + and self.modality == "video" + and "time_instruction" in locals() + ): + contexts = f"{time_instruction}\n{contexts}"Same optional cleanup as suggested for the async path.
294-294: Attribute error: use_rank(defined) instead ofrank(undefined)
self.rankisn’t set;_rankis. Current code will raiseAttributeError.Apply:
- pbar = tqdm(total=len(requests), disable=(self.rank != 0), desc="Model Responding") + pbar = tqdm( + total=len(requests), + disable=(self._rank != 0), + desc="Model Responding", + )
🧹 Nitpick comments (2)
lmms_eval/models/simple/srt_api.py (2)
287-289: Loguru formatting misuse; arguments ignored
loguruexpects{}placeholders. Current calls won’t print values.Apply:
- eval_logger.info("Question:", contexts) - eval_logger.info("Answer:", response_text) + eval_logger.info("Question: {}", contexts) + eval_logger.info("Answer: {}", response_text)
121-128: Type hints and public docstrings missing (repo guidelines)File lacks type hints and docstrings for public APIs (e.g.,
encode_image,encode_video,generate,generate_sync,generate_until). Please add them to comply with repo rules.I can draft precise type annotations and concise docstrings if helpful.
Also applies to: 129-159, 167-176, 196-205, 257-266, 320-326
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lmms_eval/models/simple/srt_api.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Type hints are required for all Python code
Public APIs must have docstrings
Maximum line length is 88 characters
Use PEP 8 naming: snake_case for functions/variables
Class names must use PascalCase
Constants should be in UPPER_SNAKE_CASE
Use f-strings for string formatting
Use early returns to avoid nested conditions
Use descriptive names; prefix handler functions with 'handle'
Prefer constants over functions where possible
Prefer functional, immutable approaches when not verbose
Define composing (higher-level) functions before their components
Mark issues in existing code with TODO: prefix in comments
Use functional and stateless approaches where they improve clarity
Use Ruff to enforce: import sorting (I001) and no unused imports
For long strings, wrap using parentheses rather than backslashes
Format long function calls over multiple lines with proper indentation
Split long import lists across multiple lines
Use Pyright type checking: add explicit None checks for Optional values
Use Pyright type narrowing for strings where applicable
Use Ruff (via pre-commit) to format and lint Python files
Document public APIs and test thoroughly
Files:
lmms_eval/models/simple/srt_api.py
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lmms_eval/models/simple/srt_api.py (1)
218-224: Avoid blocking sleep in async path (use asyncio.sleep).time.sleep blocks the event loop in generate(), degrading concurrency.
- if attempt < 4: - time.sleep(NUM_SECONDS_TO_SLEEP) + if attempt < 4: + await asyncio.sleep(NUM_SECONDS_TO_SLEEP)
🧹 Nitpick comments (2)
lmms_eval/models/simple/srt_api.py (2)
185-189: Good gating; move time_instruction inside try, wrap to 88 cols, and fix spacing.Keeps variables in-scope only on success, conforms to line-length rule, and fixes ".Please" spacing. The else branch is redundant.
Confirm there’s at most one video per request; otherwise this instruction reflects only the last processed video.
- time_instruction = f"The video lasts for {video_time:.2f} seconds, and {len(frames)} frames are uniformly sampled from it. These frames are located at {frame_time}.Please answer the following questions related to this video." - if self.add_time_instruction and self.modality == "video" and imgs is not None: - contexts = f"{time_instruction}\n{contexts}" - else: - contexts = f"{contexts}" + if self.add_time_instruction and self.modality == "video" and imgs is not None: + contexts = f"{time_instruction}\n{contexts}"And inside the try block above:
try: - frames, frame_time, video_time = self.encode_video(visual, self.max_frames_num) - imgs.extend(frames) + frames, frame_time, video_time = self.encode_video( + visual, self.max_frames_num + ) + imgs.extend(frames) + time_instruction = ( + f"The video lasts for {video_time:.2f} seconds, and " + f"{len(frames)} frames are uniformly sampled from it. " + f"These frames are located at {frame_time}. " + "Please answer the following questions related to this video." + )
246-251: Mirror the same string wrap/spacing and drop redundant else in generate_sync.Consistent formatting, 88-col compliance, and cleaner control flow.
- time_instruction = f"The video lasts for {video_time:.2f} seconds, and {len(frames)} frames are uniformly sampled from it. These frames are located at {frame_time}.Please answer the following questions related to this video." - if self.add_time_instruction and self.modality == "video" and imgs is not None: - contexts = f"{time_instruction}\n{contexts}" - else: - contexts = f"{contexts}" + if self.add_time_instruction and self.modality == "video" and imgs is not None: + contexts = f"{time_instruction}\n{contexts}"And inside the try block above:
try: - frames, frame_time, video_time = self.encode_video(visual, self.max_frames_num) - imgs.extend(frames) + frames, frame_time, video_time = self.encode_video( + visual, self.max_frames_num + ) + imgs.extend(frames) + time_instruction = ( + f"The video lasts for {video_time:.2f} seconds, and " + f"{len(frames)} frames are uniformly sampled from it. " + f"These frames are located at {frame_time}. " + "Please answer the following questions related to this video." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lmms_eval/models/simple/srt_api.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Type hints are required for all Python code
Public APIs must have docstrings
Maximum line length is 88 characters
Use PEP 8 naming: snake_case for functions/variables
Class names must use PascalCase
Constants should be in UPPER_SNAKE_CASE
Use f-strings for string formatting
Use early returns to avoid nested conditions
Use descriptive names; prefix handler functions with 'handle'
Prefer constants over functions where possible
Prefer functional, immutable approaches when not verbose
Define composing (higher-level) functions before their components
Mark issues in existing code with TODO: prefix in comments
Use functional and stateless approaches where they improve clarity
Use Ruff to enforce: import sorting (I001) and no unused imports
For long strings, wrap using parentheses rather than backslashes
Format long function calls over multiple lines with proper indentation
Split long import lists across multiple lines
Use Pyright type checking: add explicit None checks for Optional values
Use Pyright type narrowing for strings where applicable
Use Ruff (via pre-commit) to format and lint Python files
Document public APIs and test thoroughly
Files:
lmms_eval/models/simple/srt_api.py
* fix bug * fix: Correct time_instruction variable name and add modality check for video context --------- Co-authored-by: Bo Li <drluodian@gmail.com>
* fix bug * fix: Correct time_instruction variable name and add modality check for video context --------- Co-authored-by: Bo Li <drluodian@gmail.com>
2 participants
Fix bug in
srt_api.py, after fixing it, the problem in #823 is solved.Summary by CodeRabbit
Bug Fixes
Chores