-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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 for issue #19933: langchain_huggingface #25136
base: master
Are you sure you want to change the base?
Conversation
… in ChatML format instead of the formatted string
…to 'List[List[dict[str, str]]]' to reflect ChatML input
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@efriis whats the status on this? |
@ccurme Is there anything i still need to do? |
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.
Could we add some tests to demonstrate the updated behavior?
@@ -269,6 +270,7 @@ def _generate( | |||
# Process batch of prompts | |||
responses = self.pipeline( | |||
batch_prompts, | |||
return_full_text=False, |
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 do this in a separate issue / PR?
This partially fixes issue #19933 in which Langchain was not taking the taking steps to apply the correct formatting for different chat models.
The change involves passing the prompt in the ChatML format of the pipeline. This way the pipeline can automatically format accordingly.
In addition, the argument
return_full_text=False
is being passed into the pipeline inhuggingace_pipeline.py
. This causes the pipeline to only return the newly generated text whereas the default behaviour is to return the entire chat history which we do not need. The output can then be parsed back into anAIMessage
.This still only works when
ChatHuggingFace
is used. If eithermodel_id
ortokenizer
are not set when initialisingChatHuggingFace
, it defaults togpt2
tokenizer which is what caused the issue in the first place. The changes I have made correctly handle parsing the model response back intoAIMessage
. The initial issue still might need to be investigated but doing this should work as the tokenizer should be picked up from the pipeline passed intoHuggingFacePipeline
.Sorry if my language isn't clear. It is my first time contributing to any OSS project. If you have any questions or clarification is need, please say so! 😊
Edit: Did some more testing and my changes also fix the iisue the issue all together instead of partially!
model_id
ortokenizer
do not need to be set anymore.