[spec-v2] (WIP) add constrained decoding test #12635
Conversation
Summary of ChangesHello @hnyls2002, 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 introduces a new test suite designed to verify the constrained decoding capabilities when utilizing the EAGLE speculative decoding algorithm within the Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a base test class for constrained decoding tests with EAGLE speculative decoding. My review identifies a critical issue that would prevent the tests from running due to incorrect argument types for the server launch process. I have also included a suggestion to rename the test base class to align with common testing practices and prevent the test runner from discovering this base class, which contains no tests.
| "--speculative-num-steps", | ||
| cls.spec_steps, | ||
| "--speculative-eagle-topk", | ||
| cls.spec_topk, | ||
| "--speculative-num-draft-tokens", | ||
| cls.spec_draft_tokens, |
There was a problem hiding this comment.
The command-line arguments for the server launch must all be strings. The integer values for spec_steps, spec_topk, and spec_draft_tokens are not converted to strings, which will cause a TypeError when the server process is launched. They should be converted using str().
| "--speculative-num-steps", | |
| cls.spec_steps, | |
| "--speculative-eagle-topk", | |
| cls.spec_topk, | |
| "--speculative-num-draft-tokens", | |
| cls.spec_draft_tokens, | |
| "--speculative-num-steps", | |
| str(cls.spec_steps), | |
| "--speculative-eagle-topk", | |
| str(cls.spec_topk), | |
| "--speculative-num-draft-tokens", | |
| str(cls.spec_draft_tokens), |
| ) | ||
|
|
||
|
|
||
| class TestEagleServerBase(CustomTestCase, TestRegexConstrainedMixin): |
There was a problem hiding this comment.
To prevent the unittest test runner from discovering this base class (which contains no tests), it's a good practice to not prefix its name with Test. I suggest renaming it to EagleServerTestBase. Subclasses intended for testing can then inherit from it and be named with a Test prefix.
| class TestEagleServerBase(CustomTestCase, TestRegexConstrainedMixin): | |
| class EagleServerTestBase(CustomTestCase, TestRegexConstrainedMixin): |
No description provided.