Skip to content

Conversation

@Flechman
Copy link
Contributor

@Flechman Flechman commented Oct 5, 2025

Purpose

This adds test to #26231

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Rémi Delacourt <remi@mistral.ai>
Copy link
Contributor

@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 provides a bugfix for padded speculative decoding, specifically within the prepare_next_token_ids_padded function. The change removes a faulty special case for single-token generation (max_gen_len == 1) that did not correctly handle discarded requests. This could have resulted in invalid token IDs being used in subsequent steps. The new implementation generalizes the validity masking logic, making it simpler, more robust, and correct for all scenarios. The fix is sound and improves the correctness of the speculative decoding implementation.

Comment on lines 525 to 527
valid_mask = (valid_sampled_token_ids_gpu != -1) & (
valid_sampled_token_ids_gpu < gpu_input_batch.vocab_size
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This simplification is a great improvement. By removing the special case for max_gen_len == 1, the code is now more robust. The previous logic didn't account for discarded requests when max_gen_len == 1, which could lead to using an invalid token ID of -1. This unified approach correctly handles all cases.

@Flechman Flechman changed the title [Bugfix] Padded Specdec with Chunked Prefill [Bugfix] Padded Eagle Specdec with Chunked Prefill Oct 6, 2025
Signed-off-by: Rémi Delacourt <remi@mistral.ai>
@benchislett
Copy link
Collaborator

This bug has been fixed in #26231. I think it would still be nice to merge updated tests, so please update your PR with the fix from main if you wish to continue.

@Flechman
Copy link
Contributor Author

Flechman commented Oct 7, 2025

@benchislett sounds good! Done.

Signed-off-by: Rémi Delacourt <remi@mistral.ai>
Copy link
Collaborator

@benchislett benchislett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benchislett benchislett added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 8, 2025
@benchislett benchislett enabled auto-merge (squash) October 8, 2025 17:01
auto-merge was automatically disabled October 13, 2025 14:36

Head branch was pushed to by a user without write access

@Flechman
Copy link
Contributor Author

@benchislett I enforced the test to run on GPUs. It passes locally with 4xH100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed speculative-decoding v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants