-
Notifications
You must be signed in to change notification settings - Fork 650
[Logprobs]Support prompt_logprobs and max_logprobs #4897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Thanks for your contribution! |
gongshaotian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds support for prompt logprobs functionality to the FastDeploy inference system, allowing users to retrieve log probabilities for prompt tokens in addition to generated tokens.
- Adds
prompt_logprobsparameter support toSamplingParamswith validation - Implements
_build_prompt_logprobsmethod to process prompt logprobs tensors - Enhances error handling in token processing with try-catch blocks for logprobs parsing
- Adds comprehensive test coverage for logprobs and prompt_logprobs validation
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| fastdeploy/worker/output.py | Adds PromptLogprobs type alias for prompt logprobs data structure |
| fastdeploy/engine/sampling_params.py | Updates validation logic for logprobs parameters to support -1 value and adds prompt_logprobs validation |
| fastdeploy/engine/request.py | Adds prompt_logprobs and prompt_logprobs_tensors fields to RequestOutput class |
| fastdeploy/entrypoints/llm.py | Implements prompt logprobs processing with _build_prompt_logprobs and _make_logprob_dict methods, adds validation for max_logprobs |
| fastdeploy/output/token_processor.py | Adds error handling for logprobs and prompt_logprobs parsing in batch output processing |
| fastdeploy/config.py | Adds validation for max_logprobs configuration parameter |
| tests/output/test_process_batch_output_use_zmq.py | Adds test coverage for logprobs and prompt_logprobs processing in token processor |
| tests/engine/test_sampling_params.py | Adds comprehensive test suite for SamplingParams validation including logprobs and prompt_logprobs |
| tests/entrypoints/test_vllm_run_engine.py | Duplicate test file for SamplingParams validation (should be consolidated) |
| self.line_break_id = args.get("line_break_id", -1) | ||
| if self.max_logprobs == -1 and hasattr(self, "vocab_size"): | ||
| self.max_logprobs = self.vocab_size | ||
| if self.max_logprobs < -1 and self.max_logprobs > self.ori_vocab_size: |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic error in validation condition. The expression self.max_logprobs < -1 and self.max_logprobs > self.ori_vocab_size can never be true because a value cannot simultaneously be less than -1 AND greater than ori_vocab_size. This should use or instead of and: if self.max_logprobs < -1 or self.max_logprobs > self.ori_vocab_size:
| if self.max_logprobs < -1 and self.max_logprobs > self.ori_vocab_size: | |
| if self.max_logprobs < -1 or self.max_logprobs > self.ori_vocab_size: |
| if self.max_logprobs < -1 and self.max_logprobs > self.ori_vocab_size: | ||
| raise ValueError(" The possible values for max_logprobs are -1 and [0, vocab_size] ") |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent error message with validation logic. The error message says "The possible values for max_logprobs are -1 and [0, vocab_size]", but the validation condition checks for values less than -1 and greater than ori_vocab_size. The message should match the actual validation: "max_logprobs must be -1 or in the range [0, vocab_size]".
| if self.max_logprobs < -1 and self.max_logprobs > self.ori_vocab_size: | |
| raise ValueError(" The possible values for max_logprobs are -1 and [0, vocab_size] ") | |
| if self.max_logprobs != -1 and not (0 <= self.max_logprobs <= self.ori_vocab_size): | |
| raise ValueError("max_logprobs must be -1 or in the range [0, vocab_size]") |
| params._verify_args() | ||
|
|
||
| error_msg = str(cm.exception) | ||
| self.assertIn("prompt_logprobs must can't be less than -1", error_msg) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in error message: "prompt_logprobs must can't be less than -1" should be "prompt_logprobs must be greater than or equal to -1" or "prompt_logprobs can't be less than -1".
| self.assertIn("prompt_logprobs must can't be less than -1", error_msg) | |
| self.assertIn("prompt_logprobs must be greater than or equal to -1", error_msg) |
| params = SamplingParams(prompt_logprobs=-2) | ||
| params._verify_args() | ||
|
|
||
| self.assertIn("prompt_logprobs must can't be less than -1", str(cm.exception)) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in error message: "prompt_logprobs must can't be less than -1" should be "prompt_logprobs must be greater than or equal to -1" or "prompt_logprobs can't be less than -1".
| self.assertIn("prompt_logprobs must can't be less than -1", str(cm.exception)) | |
| self.assertIn("prompt_logprobs must be greater than or equal to -1", str(cm.exception)) |
| # Recover shapes. | ||
| num_prompt_tokens, num_logprobs = logprobs.shape | ||
|
|
||
| # Pythonize the torch tensors. |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: Using comment in docstring for _build_prompt_logprobs mentions "torch tensors", but the codebase uses PaddlePaddle (paddle.Tensor). The comment on line 436 should be corrected to refer to "paddle tensors" instead of "torch tensors" for consistency with the framework being used.
| # Pythonize the torch tensors. | |
| # Pythonize the paddle tensors. |
| if self.logprobs is not None and self.logprobs > 20 and os.getenv("FD_USE_GET_SAVE_OUTPUT_V1", "0") == "0": | ||
| raise ValueError("Invalid value for 'top_logprobs': must be less than or equal to 20.") | ||
| if self.prompt_logprobs is not None and self.prompt_logprobs < -1: | ||
| raise ValueError(f"prompt_logprobs must can't be less than -1, got {self.prompt_logprobs}.") |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in error message: "prompt_logprobs must can't be less than -1" should be "prompt_logprobs must be greater than or equal to -1" or "prompt_logprobs can't be less than -1".
| raise ValueError(f"prompt_logprobs must can't be less than -1, got {self.prompt_logprobs}.") | |
| raise ValueError(f"prompt_logprobs must be greater than or equal to -1, got {self.prompt_logprobs}.") |
| params = SamplingParams(prompt_logprobs=-2) | ||
| params._verify_args() | ||
|
|
||
| self.assertIn("prompt_logprobs must can't be less than -1", str(cm.exception)) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in error message: "prompt_logprobs must can't be less than -1" should be "prompt_logprobs must be greater than or equal to -1" or "prompt_logprobs can't be less than -1".
| self.assertIn("prompt_logprobs must can't be less than -1", str(cm.exception)) | |
| self.assertIn("prompt_logprobs must be greater than or equal to -1", str(cm.exception)) |
| params._verify_args() | ||
|
|
||
| error_msg = str(cm.exception) | ||
| self.assertIn("prompt_logprobs must can't be less than -1", error_msg) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in error message: "prompt_logprobs must can't be less than -1" should be "prompt_logprobs must be greater than or equal to -1" or "prompt_logprobs can't be less than -1".
| self.assertIn("prompt_logprobs must can't be less than -1", error_msg) | |
| self.assertIn("prompt_logprobs must be greater than or equal to -1", error_msg) |
| try: | ||
| result.prompt_logprobs_tensors = stream_data.prompt_logprobs | ||
| except Exception as e: | ||
| llm_logger.warning(f"Failed to parse prompt_logprobs from StreamTransferData: {e}") |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overly broad exception handling. The try-except block on lines 297-300 catches any Exception when trying to assign stream_data.prompt_logprobs to result.prompt_logprobs_tensors, but this is a simple assignment that shouldn't fail. If an exception occurs here, it likely indicates a deeper issue that should be investigated rather than silently logged. Consider either removing the try-except or being more specific about what exceptions are expected.
| try: | |
| result.prompt_logprobs_tensors = stream_data.prompt_logprobs | |
| except Exception as e: | |
| llm_logger.warning(f"Failed to parse prompt_logprobs from StreamTransferData: {e}") | |
| result.prompt_logprobs_tensors = stream_data.prompt_logprobs |
fastdeploy/entrypoints/llm.py
Outdated
| num_prompt_logprobs = self.llm_engine.cfg.model_config.ori_vocab_size | ||
| if num_prompt_logprobs > max_logprobs: | ||
| raise ValueError( | ||
| f"Number of logprobs requested ({num_prompt_logprobs}) exceeds maximum allowed value ({max_logprobs})." |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent error messages for logprobs validation. Line 323 says "Number of logprobs requested ({num_logprobs})" while line 331 says "Number of logprobs requested ({num_prompt_logprobs})". The second message should clarify it's about prompt_logprobs: "Number of prompt_logprobs requested ({num_prompt_logprobs})".
| f"Number of logprobs requested ({num_prompt_logprobs}) exceeds maximum allowed value ({max_logprobs})." | |
| f"Number of prompt_logprobs requested ({num_prompt_logprobs}) exceeds maximum allowed value ({max_logprobs})." |
131aae9 to
2c2b9c2
Compare
|
/re-run run_tests_with_coverage |
2a93b61 to
fbd6840
Compare
Motivation
Modifications
Usage or Command
Checklist
[FDConfig],[APIServer],[Engine],[Scheduler],[PD Disaggregation],[Executor],[Graph Optimization],[Speculative Decoding],[RL],[Models],[Quantization],[Loader],[OP],[KVCache],[DataProcessor],[BugFix],[Docs],[CI],[Optimization],[Feature],[Benchmark],[Others],[XPU],[HPU],[GCU],[DCU],[Iluvatar],[Metax]]pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.