test: add unit tests for managers/utils.py and template_manager.py#21227
test: add unit tests for managers/utils.py and template_manager.py#21227Xio-Shark wants to merge 3 commits intosgl-project:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the test coverage and reliability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a solid suite of unit tests for template_manager.py and utils.py, significantly improving test coverage for the managers module. The tests are well-structured and cover a good range of scenarios, including normal operation, boundary conditions, and error cases. I have a couple of minor suggestions to improve the clarity and maintainability of the new tests.
| mock_tm = MagicMock() | ||
| with self.assertRaises(ValueError): | ||
| self.tm._select_named_template({}, mock_tm) |
There was a problem hiding this comment.
The variable mock_tm is used to mock a TokenizerManager, but its name could be confused with self.tm, which is an instance of TemplateManager. For better readability, consider renaming mock_tm to mock_tokenizer_manager throughout the test classes that use it (e.g., TestSelectNamedTemplate and TestLoadExplicitChatTemplate).
| mock_tm = MagicMock() | |
| with self.assertRaises(ValueError): | |
| self.tm._select_named_template({}, mock_tm) | |
| mock_tokenizer_manager = MagicMock() | |
| with self.assertRaises(ValueError): | |
| self.tm._select_named_template({}, mock_tokenizer_manager) |
| ) | ||
| result = get_alloc_len_per_decode(args) | ||
| # page_size > 1 but topk == 1, so no error | ||
| self.assertEqual(result, max(3, 5)) |
There was a problem hiding this comment.
It's a good practice in unit testing to assert against explicit, pre-calculated values rather than expressions that replicate the implementation's logic. This makes the test's intent clearer and decouples it from the implementation details.
| self.assertEqual(result, max(3, 5)) | |
| self.assertEqual(result, 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1da11d53c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| from sglang.test.ci.ci_register import register_cpu_ci | ||
|
|
||
| register_cpu_ci(est_time=5, suite="stage-a-cpu-only") |
There was a problem hiding this comment.
Register these unit tests in the CPU CI suite
The new manager tests are tagged with suite="stage-a-cpu-only", but the CPU PR workflow only runs python3 run_suite.py --hw cpu --suite stage-a-test-cpu (.github/workflows/pr-test.yml lines 929-935), and test/run_suite.py only lists stage-a-test-cpu as a per-commit CPU suite. As a result, neither this file nor the companion test_template_manager.py will execute in CI, so regressions in managers/utils.py and template_manager.py can still merge unnoticed.
Useful? React with 👍 / 👎.
…ger.py (sgl-project#20865) Add CPU-only unit tests for two modules under sglang.srt.managers/: test_utils.py (13 tests): - validate_input_length: normal pass, boundary (==max), reject, truncate, truncate order preservation, empty input - get_alloc_len_per_decode: no speculation, basic spec, draft>steps, topk==1 with large page_size, page_size>1 && topk>1 raises - GenerationBatchResult: default fields, custom fields test_template_manager.py (12 tests): - __init__: default values - _detect_reasoning_pattern: None, empty, no-think, with-think, partial - guess_chat_template_from_model_path: known model, unknown model - _select_named_template: empty dict, single, preferred, not-found, first-of-multi - _load_explicit_chat_template: invalid raises, builtin sets name
- Remove unused imports: dataclasses, patch - Break long lines to comply with line-length limit
0403f16 to
5328540
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5328540b33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def test_template_with_think_tag_returns_true(self): | ||
| template = r"<|im_start|>assistant\n<think>\n" | ||
| self.assertTrue(self.tm._detect_reasoning_pattern(template)) |
There was a problem hiding this comment.
Exercise force-reasoning detection with real newlines
load_chat_template() feeds tokenizer.chat_template straight into _detect_reasoning_pattern(), and both _load_jinja_template() and the built-in chat templates in python/sglang/lang/chat_template.py produce strings with real newline characters (for example "<|im_start|>assistant\n"), not the literal \\n sequences used in this test. In that scenario the current regex does not match, so this test would stay green while force-reasoning detection is broken for the templates users actually load.
Useful? React with 👍 / 👎.
| def test_exactly_at_limit_returns_error(self): | ||
| """Input length == max is treated as exceeding (>= check).""" | ||
| req = self._make_req(200) | ||
| result = validate_input_length( | ||
| req, max_req_input_len=200, allow_auto_truncate=False |
There was a problem hiding this comment.
Stop treating max-length prompts as out of bounds
The helper documents max_req_input_len as the maximum allowed input length, and --allow-auto-truncate is described as applying only when a request exceeds that limit. tp_worker.py also already reserves five tokens of headroom before computing max_req_input_len, so asserting that len == max_req_input_len must fail bakes the current off-by-one behavior into CI and will block a future fix that accepts prompts exactly at the published limit.
Useful? React with 👍 / 👎.
|
@ispobock Could you please add the |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Part of #20865: add CPU-only unit tests for
managers/module.test_utils.py (13 tests):
validate_input_length: normal/boundary/reject/truncateget_alloc_len_per_decode: no spec/basic spec/draft tokens/page_size errorGenerationBatchResult: default and custom fieldstest_template_manager.py (12 tests):
_detect_reasoning_pattern: None/empty/no-think/with-think/partialguess_chat_template_from_model_path: known/unknown model_select_named_template: empty/single/preferred/not-found/multi_load_explicit_chat_template: invalid raises/builtin sets