Skip to content

Conversation

mgoin
Copy link
Member

@mgoin mgoin commented Sep 23, 2025

Purpose

Fix failing "Model Executor Test" with an issue that was introduced by #24542

[2025-09-23T00:42:31Z]         for op in self.custom_ops:
[2025-09-23T00:42:31Z] >           if op[0] not in {'+', '-'} and op not in {'all', 'none'}:
[2025-09-23T00:42:31Z] E           IndexError: string index out of range
[2025-09-23T00:42:31Z]
[2025-09-23T00:42:31Z] /usr/local/lib/python3.12/dist-packages/vllm/config/compilation.py:491: IndexError

[2025-09-23T00:42:31Z] FAILED model_executor/test_enabled_custom_ops.py::test_enabled_ops[-0-False-ops_enabled0-True] - IndexError: string index out of range

https://buildkite.com/vllm/ci/builds/31919/steps/canvas?jid=019973de-a941-4e89-8d50-c2803680fbe2

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: mgoin <mgoin64@gmail.com>
Copy link
Contributor

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

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 correctly fixes a crash in CompilationConfig.__post_init__ caused by an empty string in the custom_ops list. The added check is appropriate and resolves the reported CI failure. I've noticed that the custom_op_log_check method in the same file has a similar vulnerability where it iterates over custom_ops without checking for empty strings, which could also lead to an IndexError. While this is outside the scope of the current changes, I recommend addressing this in a follow-up to improve the robustness of the code.

@mgoin mgoin added the ci-failure Issue about an unexpected test failure in CI label Sep 23, 2025
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) September 23, 2025 01:14
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 23, 2025
@mgoin
Copy link
Member Author

mgoin commented Sep 23, 2025

Resolved by #25429

@mgoin mgoin closed this Sep 23, 2025
auto-merge was automatically disabled September 23, 2025 01:36

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-failure Issue about an unexpected test failure in CI ready ONLY add when PR is ready to merge/full CI is needed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants