Skip to content
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

vicuna #13

Merged
merged 14 commits into from
Jan 3, 2024
Merged

vicuna #13

merged 14 commits into from
Jan 3, 2024

Conversation

aglick24
Copy link
Contributor

added a to_chatml function for vicuna and testing the fork, will next do a from_chatml function

@aglick24 aglick24 changed the title to_chatml for vicuna vicuna Dec 24, 2023
@aglick24
Copy link
Contributor Author

based on vicuna format coming from https://huggingface.co/junelee/wizard-vicuna-13b/discussions/1

@anjor
Copy link
Contributor

anjor commented Dec 24, 2023

@aglick24 thanks for the contribution!

It looks like there is some dispute about the precise format though -- https://huggingface.co/junelee/wizard-vicuna-13b/discussions/1#6453c515b8c58783d662bc05

Also I think you will need to import it in the init.py like so: https://huggingface.co/junelee/wizard-vicuna-13b/discussions/1#6453c515b8c58783d662bc05

@aglick24
Copy link
Contributor Author

Thanks @anjor ! What do you think about this format? On lines 393-404 https://github.com/lm-sys/FastChat/blob/main/fastchat/conversation.py and if so, should I include the separator?

@aglick24
Copy link
Contributor Author

aglick24 commented Dec 26, 2023

This seems to me to be an example of what that conversation template should look like this (using (SEP) instead of the actual separator because it disappears in the comment thread):

"A chat between a curious user and an artificial intelligence assistant. The assistant gives helpful, detailed, and polite answers to the user's questions. USER: What is ChatGPT? ASSISTANT: ChatGPT is an advanced language model developed by OpenAI. It's designed to understand and generate natural language, so it can converse with users, answer questions, and even assist with various tasks.(SEP)USER: So, it's like a super-advanced chatbot? ASSISTANT: Yes, but it can engage in broader conversations and more complex reasoning.(SEP)"

@aglick24
Copy link
Contributor Author

Following up @anjor

@anjor
Copy link
Contributor

anjor commented Dec 28, 2023

Could you fix the tests?

Also, if you could hold off until #14 is merged. It might be a good idea to implement that abstract class instead of the current implementation. There is an example implementation in #15

return SEPARATOR.join(chat_history)


if __name__ == "__main__":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove this. The test is good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, sorry forgot about it!

@koogle
Copy link
Contributor

koogle commented Dec 31, 2023

@aglick24 let us know if there is anything we can help with to get it over the line and merged?

@aglick24
Copy link
Contributor Author

aglick24 commented Jan 2, 2024

Thanks @koogle for merging main! Sorry about the delay, I assumed we were waiting on the changes to merge to main before I would keep going. Looking through it now to update it to the new format

@aglick24
Copy link
Contributor Author

aglick24 commented Jan 3, 2024

Just refactored and fixed the tests! Created version 0.0.6 to add vicuna @koogle @anjor

@@ -8,7 +8,7 @@
If a question does not make any sense, or is not factually coherent, explain why instead of answering something not correct. If you don't know the answer to a question, please don't share false information.
<</SYS>>

Hi, how are you? [/INST] Good thanks!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, thanks for catching! Autoformatter strikes again. Just reverted all of the Llama2 changes

@koogle
Copy link
Contributor

koogle commented Jan 3, 2024

@aglick24 thank you so much for this contribution, approving
feel free to press the merge button

fixes #5

@aglick24
Copy link
Contributor Author

aglick24 commented Jan 3, 2024

Screenshot 2024-01-03 at 9 40 06 AM

Thanks so much for the approval @koogle ! Unfortunately, I need write access to merge

@koogle
Copy link
Contributor

koogle commented Jan 3, 2024

Okay in that case I will merge it , thank you for the contribution

@koogle koogle merged commit 81e5107 into deployradiant:main Jan 3, 2024
1 check passed
@koogle
Copy link
Contributor

koogle commented Jan 3, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants