Skip to content

Conversation

@a120092009
Copy link
Contributor

No description provided.

@zhang-minchao
Copy link
Collaborator

/gemini review

Copy link

@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 introduces a RejectionSamplerRateController to allow fixing the speculative acceptance rate for performance debugging, controlled via an environment variable. The implementation adds a complex controller with PID logic and various heuristics. While the feature appears to be implemented as intended, the new controller class has significant code quality issues, primarily due to a large number of "magic numbers" that make the code difficult to understand, maintain, and tune. My review focuses on improving maintainability and robustness by suggesting the use of named constants and safer coding practices.

@a120092009 a120092009 force-pushed the feat/fix-spec-rates branch from 52d737a to 02b034f Compare January 6, 2026 01:18
@zhang-minchao
Copy link
Collaborator

/gemini review

Copy link

@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 introduces a feature for performance debugging of speculative decoding by allowing a fixed acceptance rate to be set via an environment variable. This is implemented through a new RejectionSamplerRateController class that is used by the RejectionSampler. The changes are well-structured, introducing a new utility function to parse environment variables and integrating the new controller cleanly. My review focuses on improving maintainability by replacing magic numbers with their corresponding named constants that are already defined in the header file.

@a120092009 a120092009 force-pushed the feat/fix-spec-rates branch from 02b034f to 6015a03 Compare January 6, 2026 07:06
@zhang-minchao zhang-minchao self-requested a review January 6, 2026 12:17
@XuZhang99 XuZhang99 merged commit be60c75 into jd-opensource:main Jan 7, 2026
15 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants