Add function to set end_id and apply chat template for GAP Triton In-Process #183
Conversation
add feature to set end_id for trtllm engine input convert
| if config.set_end_id: | ||
| payload["end_id"] = [config.tokenizer._tokenizer.eos_token_id] |
There was a problem hiding this comment.
I am a bit concerned about adding a new CLI option for this specific use cases. Since we try to avoid adding too much options to the tool, and this can be achieved through the --extra-inputs <name>:<value> as well.
cc @dyastremsky
There was a problem hiding this comment.
Yeah if this can be done via --extra-inputs, let's skip this change.
There was a problem hiding this comment.
It is moved to --extra-inputs. however, code change is still necessary unless use directly provide eos token id (instead of fetching from tokenizer). Please let me know if current approach looks good. Thanks.
| ) | ||
|
|
||
| input_group.add_argument( | ||
| "--triton-converter-apply-chat-template", |
There was a problem hiding this comment.
apply_chat_template I think could be a useful cli option to add: https://huggingface.co/docs/transformers/main/en/chat_templating
But I would go for adding it in a more generic way to support chat_template for any use cases, not just for triton in-process.
There was a problem hiding this comment.
Please correct me if I'm wrong. Is chat template only necessary to benchmark raw engines? Benchmarking through api endpoints does not need to add chat template in GAP, right?
There was a problem hiding this comment.
Yes, you're right. I guess what I wanted to say was we want to make it more generic so that the option is not limited to only triton in-process although the current use case is just triton in-process. This way we have a stronger reason to add this to our cli options.
There was a problem hiding this comment.
Got it. Does GAP have any other route to benchmark raw engines that might need chat templates? I am not familiar with the codebase and it would be great if someone from your team has some BW to help.
There was a problem hiding this comment.
The link from Hyunjae shows how chat templates are used with some APIs. The only endpoint that I know supports templates right now is the chat endpoint.
The team is bandwidth-constrained at the moment, though that could be a good addition.
| elif isinstance(r["output_ids"], int): | ||
| token_ids.append(r["output_ids"]) | ||
| else: | ||
| # for the empty first/last responses | ||
| token_ids.append(0) |
There was a problem hiding this comment.
@matthewkotila added a bug fix for empty first/last response (it will be and empty string and cause error).
| @@ -571,6 +571,13 @@ def _add_image_input_args(parser): | |||
| "If format is not selected, format of generated image is selected at random", | |||
| ) | |||
|
|
|||
| input_group.add_argument( | |||
There was a problem hiding this comment.
I agree with Hyunjae's point below. Also, the image input arg function does not seem like the right place for this.
| ) | ||
|
|
||
| input_group.add_argument( | ||
| "--triton-converter-apply-chat-template", |
There was a problem hiding this comment.
The link from Hyunjae shows how chat templates are used with some APIs. The only endpoint that I know supports templates right now is the chat endpoint.
The team is bandwidth-constrained at the moment, though that could be a good addition.
| set_end_id: bool = False | ||
|
|
||
| # whether to apply chat template in triton converter | ||
| apply_chat_template: bool = False |
There was a problem hiding this comment.
Update the comment to make this generic. You'd also want apply_chat_template to be a string if we're making it generic based on how other endpoints use chat templating.
Newline after this.
| @@ -142,3 +142,9 @@ class InputsConfig: | |||
|
|
|||
| # Seed used to generate random values | |||
| random_seed: int = DEFAULT_RANDOM_SEED | |||
|
|
|||
| # whether to set end_id in triton converter | |||
| set_end_id: bool = False | |||
There was a problem hiding this comment.
Is this still necessary? We don't want endpoint-specific fields in inputs_config.py.
| @@ -68,6 +68,9 @@ def __call__(self, text, **kwargs) -> "BatchEncoding": | |||
| def encode(self, text, **kwargs) -> List[int]: | |||
| self._encode_args.update(kwargs) | |||
| return self._tokenizer.encode(text, **self._encode_args) | |||
|
|
|||
| def apply_chat_template(self, text) -> List[int]: | |||
| return self._tokenizer.encode(self._tokenizer.apply_chat_template([{"role": "user", "content": text}], tokenize=False), add_special_tokens=False) | |||
There was a problem hiding this comment.
We can't have a TRT-LLM-specific chat template in the tokenizer class. We should have the user provide the template, then the tokenizer applies it if has one.
If we're making this endpoint-specific, then it should exist solely in the converter as an extra arg. As long as it doesn't affect metrics, which I think it shouldn't for TRT-LLM in-process. (CC: @nv-hwoo)
| @@ -82,4 +85,7 @@ def _add_request_params(self, payload: Dict, config: InputsConfig) -> None: | |||
| payload["min_length"] = [num_tokens] | |||
|
|
|||
| for key, value in config.extra_inputs.items(): | |||
| payload[key] = [value] | |||
| if key == "triton_converter_set_end_id" and value: | |||
There was a problem hiding this comment.
You could probably shorten this key, if you wanted. e.g. "set_end_id"
Add two functions for GAP Triton In-Process: