Fix echo/logprob OpenAI completion bug#3441
Conversation
|
@ywang96 Thanks for the recommendations! |
dylanwhawk
force-pushed
the
echo_logprob_bug
branch
from
c05cd50 to
0de7e79
Compare
| if top_logprobs is None or top_logprobs[i] is None: | ||
| token = self.tokenizer.decode(token_id) | ||
| logprobs.tokens.append(token) | ||
| logprobs.token_logprobs.append(None) | ||
| logprobs.top_logprobs.append(None) |
There was a problem hiding this comment.
can you explain when would this be the case and why is the decode needed?
There was a problem hiding this comment.
This is the case when echo == True and logprobs > 0 for the first prompt token because there is no sampling metadata (top_logprobs[i]) about it. The decode is needed to add the token to the logprob token list because the sampling metadata normally has it.
There was a problem hiding this comment.
The top_logprobs is None was actually never possible so I removed it
| prompt_ids, prompt_text = self._validate_prompt_and_tokenize( | ||
| request, prompt=prompt) |
There was a problem hiding this comment.
I looked through the diff but I can't find why is prompt_text needed
There was a problem hiding this comment.
On line 79, prompt was what was given by user (either text or token), but generate expected it to be text for handling echo
There was a problem hiding this comment.
Ah can you add a comment
simon-mo
left a comment
simon-mo
left a comment
There was a problem hiding this comment.
plz fix merge conflict
| prompt_ids, prompt_text = self._validate_prompt_and_tokenize( | ||
| request, prompt=prompt) |
There was a problem hiding this comment.
Ah can you add a comment
| if step_top_logprobs is None: | ||
| token = self.tokenizer.decode(token_id) | ||
| logprobs.tokens.append(token) | ||
| logprobs.token_logprobs.append(None) | ||
| logprobs.top_logprobs.append(None) | ||
| else: | ||
| token_logprob = None | ||
| token = step_top_logprobs[token_id].decoded_token | ||
| logprobs.tokens.append(token) | ||
| logprobs.token_logprobs.append(token_logprob) | ||
| token_logprob = step_top_logprobs[token_id].logprob | ||
| token = step_top_logprobs[token_id].decoded_token | ||
| logprobs.tokens.append(token) | ||
| logprobs.token_logprobs.append(token_logprob) | ||
|
|
||
| if num_output_top_logprobs: | ||
| logprobs.top_logprobs.append({ | ||
| p.decoded_token: p.logprob | ||
| for i, p in step_top_logprobs.items() | ||
| } if step_top_logprobs else None) | ||
|
|
There was a problem hiding this comment.
I don't think we need to call tokenizer here? cc @Yard1
| input_text = prompt if prompt is not None else self.tokenizer.decode( | ||
| prompt_ids) |
There was a problem hiding this comment.
I don't think we should be unconditionally detokenizing the prompt ids in this case. It would be better to do this if/where it's actually needed. At minimum only if echo is specified.
Co-authored-by: Dylan Hawk <dylanwawk@gmail.com>
|
I've noticed flaky behaviour in Edit: Hopefully #3512 would fix this issue. |
Co-authored-by: Dylan Hawk <dylanwawk@gmail.com>
5 participants
Fixes #2703 and allows echo to be used with a list of tokens