Skip to content

test(vlm): fix always-true image assertion - #31932

Closed
mickqian wants to merge 2 commits into
sgl-project:mainfrom
mickqian:codex/fix-vlm-test-truthy-assert
Closed

mickqian wants to merge 2 commits into
sgl-project:mainfrom
mickqian:codex/fix-vlm-test-truthy-assert

Conversation

@mickqian

@mickqian mickqian commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • make the person branch in the shared VLM single-image semantic check evaluate membership instead of a truthy string literal
  • prevent test_vision_openai_server from accepting responses that miss all person/man/driver concepts

Validation

  • python3 -m py_compile python/sglang/test/vlm_utils.py
  • AST audit confirms there are no remaining bare truthy string operands in assert-or expressions under the VLM test utilities

CI States

Latest PR Test (Base): ⏳ Run #30347730447
Latest PR Test (Extra): ❌ Run #30347730327

@github-actions github-actions Bot added the Multi-modal multi-modal language model label Jul 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes a logical bug in the assertion within verify_single_image_response where the string "person" was evaluated as a truthy value instead of checking its membership in text. The reviewer suggested improving the robustness of this check by making it case-insensitive.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

# `driver` is for gemma-3-it
assert (
"man" in text or "person" or "driver" in text
"man" in text or "person" in text or "driver" in text

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To make the assertion more robust against variations in model capitalization (e.g., if the model starts the response with 'Man' or 'Person'), consider performing a case-insensitive check by converting text to lowercase.

Suggested change
"man" in text or "person" in text or "driver" in text
"man" in text.lower() or "person" in text.lower() or "driver" in text.lower()

@mickqian

Copy link
Copy Markdown
Collaborator Author

/tag-and-rerun-ci

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Multi-modal multi-modal language model run-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant