-
Notifications
You must be signed in to change notification settings - Fork 263
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
Fix: Total number of tokens in each stop word should be 1 #1892
Conversation
# @add_start_docstrings(STOPPING_CRITERIA_INPUTS_DOCSTRING) | ||
def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor, **kwargs) -> bool: | ||
# Create a tensor from the stop_sequence | ||
stop_sequence_tensor = torch.tensor(self.stop_sequence, device=input_ids.device, dtype=input_ids.dtype) |
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.
This loads a tensor onto the model's device at every step, which seems slow; is there any way we can just load the tensor once in the constructor? Can you get the device and dtype from the model?
Separately, would you have an idea about how large the performance impact is?
cc @dlwh for PyTorch expertise - David would you have any advice here?
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.
Sorry I have not resolved the issue yet for poor knowledge on transformers library. In terms of efficiency, there are different reductions on different datasets and adapters. Usually it costs no more than 5 times the original time.
Thank for the PR, this looks very useful! |
output = self.model.generate( | ||
**encoded_input, | ||
**relevant_raw_request, | ||
stopping_criteria=stopping_criteria if len(stop_sequence_ids.input_ids[0]) > 1 else None, |
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.
However, the good news is that only few model have to use stopping_criteria.
I’d be surprised if this were a bottleneck or even measurable. But I could
be wrong. Putting a scalar on a gpu is probably not that big of a deal.
If it is a concern, I imagine you could just cache it.
…On Thu, Oct 12, 2023 at 2:22 PM Yifan Mai ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/helm/proxy/clients/huggingface_client.py
<#1892 (comment)>:
> @@ -35,6 +41,21 @@ def resolve_alias(model_name: str) -> str:
return _MODEL_NAME_ALIASES.get(model_name, model_name)
+class StopAtSpecificTokenCriteria(StoppingCriteria):
+ def __init__(self, stop_sequence: List[int] = None):
+ super().__init__()
+ self.stop_sequence = stop_sequence
+
+ # @add_start_docstrings(STOPPING_CRITERIA_INPUTS_DOCSTRING)
+ def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor, **kwargs) -> bool:
+ # Create a tensor from the stop_sequence
+ stop_sequence_tensor = torch.tensor(self.stop_sequence, device=input_ids.device, dtype=input_ids.dtype)
This loads a tensor onto the model's device at every step, which seems
slow; is there any way we can just load the tensor once in the constructor?
Can you get the device and dtype from the model?
Separately, would you have an idea about how large the performance impact
is?
cc @dlwh <https://github.com/dlwh> for PyTorch expertise - David would
you have any advice here?
—
Reply to this email directly, view it on GitHub
<#1892 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACLIJF376G4ZRZZKJKET3X7BNPXAVCNFSM6AAAAAA53M7PSCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNZVGIZDMNBSGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Maybe you are right. I think you can test it on your machine for further conclusion. :) |
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.
Test breakages look unrelated; I will address in a separate PR. |
I use StoppingCriteria instead of eos_token_id when total number of tokens in each stop word is more than 1, which would allow helm to use stop sequences of multiple tokens.
It should fix #1501 and #1782 .
Notice: multiple tokens in stop word may lead to reduced efficiency