-
Notifications
You must be signed in to change notification settings - Fork 31
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
Align Tokenizer in JetStream #40
Changes from 2 commits
c97c4fd
8016fac
1e3ddc6
3985e9b
24f9d74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
nltk | ||
evaluate | ||
rouge-score | ||
rouge-score | ||
tqdm |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,10 @@ message DecodeRequest { | |
int32 max_tokens = 4; | ||
} | ||
message DecodeResponse { | ||
// List of responses, one per sample. | ||
repeated string response = 1; | ||
// List of responses, one per sample. The list size depends on text generation strategy the engine used. | ||
repeated RepeatedTokenIds response = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add your deleted field number to the reserved list, it may messes up deserialization. Here is the reference: https://protobuf.dev/programming-guides/dos-donts/#reserve-tag-numbers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not keep both and let the user choose whether she wants ids or string? |
||
} | ||
message RepeatedTokenIds { | ||
// List of token ids, one list per sample. When speculative decoding is disabled, the list size should be 1; When speculative decoding is enabled, the list size should be >= 1. | ||
repeated int32 token_ids = 1; | ||
} |
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.
Can we still keep a str as option? The internal keep both text and token id.
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.
I guess we don't want to decode it to str (or piece) in jetstream, since it would have some off in the final result.
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.
Great! Thanks for making the changes!