Tool parser regex timeout handling - #18960
Conversation
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
russellb
left a comment
russellb
left a comment
There was a problem hiding this comment.
great idea! just one minor suggestion
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
| VLLM_NIXL_SIDE_CHANNEL_PORT: int = 5557 | ||
| VLLM_ALL2ALL_BACKEND: str = "naive" | ||
| VLLM_MAX_TOKENS_PER_EXPERT_FP4_MOE: int = 163840 | ||
| VLLM_TOOL_PARSE_REGEX_TIMEOUT: int = 5 |
There was a problem hiding this comment.
We should include the units (assuming secs?) somewhere.. probably in the env var name but if not in the comments.
There was a problem hiding this comment.
Especially given that it's overridable, I feel like the default should be shorter (e.g. 1 or 2 seconds?) since time spent in this is going to block concurrent requests too.
| if not (self.TOOL_CALL_REGEX.match(model_output)): | ||
|
|
||
| try: | ||
| if not (self.TOOL_CALL_REGEX.match( |
There was a problem hiding this comment.
nit: superfluous parentheses (though there were already there before this change)
| logger.warning( | ||
| "Regex timeout occurred when matching tool call pattern.") |
There was a problem hiding this comment.
May be good to include the problematic input in this log, at least if debug is enabled? (if debug isn't enabled we might want to avoid logging user data).
| return ExtractedToolCallInformation(tools_called=False, | ||
| tool_calls=[], | ||
| content=model_output) |
There was a problem hiding this comment.
nit: maybe have a bool variable which is set in both the non-match and timeout cases to avoid duplicate return statements?
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
|
@njhill thanks for the careful review, I think I've addressed everything! |
github-actions
Bot
added
the
ready
Signed-off-by: Will Eaton <weaton@redhat.com> Signed-off-by: amit <amit.man@gmail.com>
Signed-off-by: Will Eaton <weaton@redhat.com> Signed-off-by: amit <amit.man@gmail.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
3 participants
In #18454 we moved off of the std library regex library in an attempt to prevent catastrophic backtracking.
The replacement
regexlibrary also supports a timeout flag, this PR adds a global timeout (making use of the new flag) for complex tool parsers as an extra layer of protection against malicious input.