[feat] support whitespace control for structural tag#660
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces whitespace control parameters (any_whitespace and max_whitespace_cnt) to the structural tag compilation and grammar generation interfaces to prevent state explosion. Feedback suggests optimizing the C++ style_to_grammar_converter map by replacing std::function with raw function pointers to avoid dynamic allocation overhead. Additionally, it is recommended to validate max_whitespace_cnt in Python to ensure it is a positive integer before passing it to the C++ backend.
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.
| const static std::unordered_map< | ||
| std::string, | ||
| std::function<std::string(const std::string&, bool, std::optional<int>)>> | ||
| style_to_grammar_converter = { |
There was a problem hiding this comment.
Using std::function in a const static map introduces unnecessary overhead due to dynamic allocation and virtual dispatch. Since the lambdas do not capture any variables, they can be implicitly converted to raw function pointers, which are more efficient.
const static std::unordered_map<\n std::string,\n std::string (*)(const std::string&, bool, std::optional<int>)>\n style_to_grammar_converter = {| any_whitespace = kwargs.pop("any_whitespace", True) | ||
| max_whitespace_cnt = kwargs.pop("max_whitespace_cnt", None) |
There was a problem hiding this comment.
Validate max_whitespace_cnt in Python to ensure it is a positive integer before passing it to the C++ backend. This prevents potential C++ fatal log crashes and provides a clearer Python ValueError to the user.
any_whitespace = kwargs.pop(\"any_whitespace\", True)\n max_whitespace_cnt = kwargs.pop(\"max_whitespace_cnt\", None)\n if max_whitespace_cnt is not None and (not isinstance(max_whitespace_cnt, int) or isinstance(max_whitespace_cnt, bool) or max_whitespace_cnt <= 0):\n raise ValueError(\"max_whitespace_cnt must be a positive integer\")| any_whitespace = kwargs.pop("any_whitespace", True) | ||
| max_whitespace_cnt = kwargs.pop("max_whitespace_cnt", None) |
There was a problem hiding this comment.
Validate max_whitespace_cnt in Python to ensure it is a positive integer before passing it to the C++ backend. This prevents potential C++ fatal log crashes and provides a clearer Python ValueError to the user.
any_whitespace = kwargs.pop(\"any_whitespace\", True)\n max_whitespace_cnt = kwargs.pop(\"max_whitespace_cnt\", None)\n if max_whitespace_cnt is not None and (not isinstance(max_whitespace_cnt, int) or isinstance(max_whitespace_cnt, bool) or max_whitespace_cnt <= 0):\n raise ValueError(\"max_whitespace_cnt must be a positive integer\")|
@Ubospica @Seven-Streams Could you please review this PR when you have time? |
| def from_structural_tag(tags: List[StructuralTagItem], triggers: List[str]) -> "Grammar": ... | ||
| def from_structural_tag( | ||
| tags: List[StructuralTagItem], | ||
| triggers: List[str], |
There was a problem hiding this comment.
We may need to mention it in the doc.
There was a problem hiding this comment.
Done. Added a short note to docs/structural_tag/tool_calling_and_reasoning.md.
|
@Seven-Streams I've addressed all the comments. Could you please take another look when you have a chance? |
@Seven-Streams @Ubospica Could you please help merge this PR manually? |
|
Hello @izhuhaoran. I discussed the function with @Ubospica today, and I'll make some modifications tomorrow, and then the PR will be merged. Thanks for your contribution! |
3b5998b to
069c208
Compare
Signed-off-by: zhuhaoran <zhuhaoran.zhr@alibaba-inc.com> use XGRAMMAR_DCHECK for the int32 overflow guard and document whitespace controls Signed-off-by: zhuhaoran <zhuhaoran.zhr@alibaba-inc.com> test. Signed-off-by: yuchuan <yuchuan.7streams@gmail.com> simplify. Signed-off-by: yuchuan <yuchuan.7streams@gmail.com>
069c208 to
902e2fb
Compare
|
@Seven-Streams Thank you for taking the time to further improve this PR. I really appreciate it. I noticed it hasn’t been merged yet, so I just wanted to check whether there are any remaining concerns or anything else needed from my side. |
Ubospica
left a comment
There was a problem hiding this comment.
LGTM! @Seven-Streams if you have time plz cross check
Some models, in bad-case generations, emit unbounded runs of whitespace, which inflates the Earley parser's state set and can blow up grammar compilation/matching.
Adds two whitespace controls —
any_whitespaceandmax_whitespace_cnt— as parameters of the structural-tag compile interface (GrammarCompiler.compile_structural_tag/Grammar.from_structural_tag), mirroringcompile_json_schema.