[None] [fix] Revert "[None] [feat] add eos_token_id in generation_config to sampling params"

[kaiyux]

Reverts #9514

Summary by CodeRabbit

• Bug Fixes
  • Improved end-of-sequence token handling for specific model configurations.
  • Enhanced stop word integration with maintained backward compatibility across model variants.
  • Refined sampling parameter processing for better generation behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce model-type specific handling for end_id and stop words in sampling parameters. For "kimi_k2" models, end_id overrides from generation_config.eos_token_id. For "qwen3_next" models, eos_token_id is conditionally appended to stop words with deduplication logic. A test prompt string is simplified.

Changes

Cohort / File(s)	Summary
Sampling Parameters Model-Type Handling tensorrt_llm/sampling_params.py	Adds model-specific logic for "kimi_k2" to override end_id from generation_config.eos_token_id. Introduces conditional handling for "qwen3_next" to append eos_token_id to stop words with deduplication (only appending tokens not already in existing stop words). Removes broad eos_token_id consumption logic, restricting it to qwen3_next path while maintaining backward compatibility.
Test Data Simplification tests/unittest/llmapi/apps/_test_trtllm_serve_top_logprobs.py	Simplifies user message prompt from "What is the capital of France? please in detail." to "What is the capital of France?" in test_chat_completion_top1_logprobs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• tensorrt_llm/sampling_params.py: Review the new conditional logic paths for model-type specific handling, particularly the deduplication logic when appending eos_token_id to existing stop words and the backward compatibility fallback behavior.
• Boundary testing: Verify that the changes correctly handle both list and non-list eos_token_id values for "qwen3_next", and that other model types are unaffected.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	The PR description is minimal, providing only a reference to the reverted PR without explaining why the revert is necessary or what issues it addresses.	Add details explaining the reason for the revert, any issues or regressions from PR #9514, and confirm test coverage validates the revert.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title indicates this is a revert of a previous feature addition, which aligns with the actual changes removing broad eos_token_id logic and replacing it with model-specific conditions.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch revert-9514-add_eos_token_generation_config_to_stop_words

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c98b25 and 5396b63.

📒 Files selected for processing (2)

• tensorrt_llm/sampling_params.py (2 hunks)
• tests/unittest/llmapi/apps/_test_trtllm_serve_top_logprobs.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Always maintain the namespace when importing in Python, even if only one class or function from a module is used (e.g., use from package.subpackage import foo and then foo.SomeClass() instead of from package.subpackage.foo import SomeClass)
Python filenames should use snake_case (e.g., some_file.py)
Python class names should use PascalCase (e.g., class SomeClass)
Python function and method names should use snake_case (e.g., def my_awesome_function():)
Python local variable names should use snake_case, with prefix k for variable names that start with a number (e.g., k_99th_percentile = ...)
Python global variables should use upper snake_case with prefix G (e.g., G_MY_GLOBAL = ...)
Python constants should use upper snake_case (e.g., MY_CONSTANT = ...)
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments should be reserved for code within a function, or interfaces that are local to a file
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description (e.g., self.x = 5 followed by """<type>: Description of 'x'""" )
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of specific errors possible instead of catching all exceptions
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block to implement the logic

Files:

• tests/unittest/llmapi/apps/_test_trtllm_serve_top_logprobs.py
• tensorrt_llm/sampling_params.py

**/*.{cpp,h,cu,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code files should contain an NVIDIA copyright header that includes the current year at the top

Files:

• tests/unittest/llmapi/apps/_test_trtllm_serve_top_logprobs.py
• tensorrt_llm/sampling_params.py

🧠 Learnings (2)

📓 Common learnings

Learnt from: samuellees
Repo: NVIDIA/TensorRT-LLM PR: 6974
File: tensorrt_llm/serve/scripts/benchmark_dataset.py:558-566
Timestamp: 2025-08-18T08:42:02.640Z
Learning: In TensorRT-LLM's RandomDataset (tensorrt_llm/serve/scripts/benchmark_dataset.py), when using --random-token-ids option, sequence length accuracy is prioritized over semantic correctness for benchmarking purposes. The encode/decode operations should use skip_special_tokens=True and add_special_tokens=False to ensure exact target token lengths.

📚 Learning: 2025-08-18T08:42:02.640Z

Learnt from: samuellees
Repo: NVIDIA/TensorRT-LLM PR: 6974
File: tensorrt_llm/serve/scripts/benchmark_dataset.py:558-566
Timestamp: 2025-08-18T08:42:02.640Z
Learning: In TensorRT-LLM's RandomDataset (tensorrt_llm/serve/scripts/benchmark_dataset.py), when using --random-token-ids option, sequence length accuracy is prioritized over semantic correctness for benchmarking purposes. The encode/decode operations should use skip_special_tokens=True and add_special_tokens=False to ensure exact target token lengths.

Applied to files:

• tensorrt_llm/sampling_params.py

🧬 Code graph analysis (1)

tensorrt_llm/sampling_params.py (2)

tensorrt_llm/bench/benchmark/__init__.py (1)

• model_type (70-71)

tests/unittest/llmapi/test_llm.py (1)

• eos_token_id (306-307)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Pre-commit Check
• GitHub Check: Analyze (python)

🔇 Additional comments (3)

tests/unittest/llmapi/apps/_test_trtllm_serve_top_logprobs.py (1)

113-113: LGTM!

The prompt simplification aligns this test with test_chat_completion_top5_logprobs (line 74), ensuring consistency across both logprobs test cases.

tensorrt_llm/sampling_params.py (2)

376-383: Verify pad_id alignment for kimi_k2 model.

The end_id is overridden from generation_config.eos_token_id for kimi_k2, but pad_id (set at line 375) remains as tokenizer.pad_token_id. Confirm this asymmetry is intentional—typically pad_id defaults to end_id when not explicitly set (as handled at lines 385-386 only if pad_id is None).

---

417-420: Verify stop word semantics for multi-token eos_token_id.

_stop_word_ids is List[List[int]] where each inner list is a token sequence. The current logic appends from_generation_stop_tokens (e.g., [100, 200]) as a single sequence, meaning generation stops only when tokens 100 AND 200 appear consecutively.

If each eos token should independently stop generation, wrap them individually:

-                if from_generation_stop_tokens:
-                    self._stop_word_ids.append(from_generation_stop_tokens)
-            else:
-                self._stop_word_ids = [generation_config.eos_token_id]
+                if from_generation_stop_tokens:
+                    self._stop_word_ids.extend([[t] for t in from_generation_stop_tokens])
+            else:
+                self._stop_word_ids = [[t] for t in generation_config.eos_token_id]

[kaiyux]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #28357 [ run ] triggered by Bot. Commit: 5396b63

[tensorrt-cicd]

PR_Github #28357 [ run ] completed with state SUCCESS. Commit: 5396b63
/LLM/main/L0_MergeRequest_PR pipeline #21694 completed with status: 'FAILURE'

[kaiyux]

/bot run --disable-fail-fast

[tensorrt-cicd]

PR_Github #28414 [ run ] triggered by Bot. Commit: 5396b63

[tensorrt-cicd]

PR_Github #28414 [ run ] completed with state SUCCESS. Commit: 5396b63
/LLM/main/L0_MergeRequest_PR pipeline #21744 completed with status: 'SUCCESS'