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

Implementing more CodeLMs #41

Merged
merged 3 commits into from
Apr 22, 2023
Merged

Conversation

yilunzhao
Copy link
Member

Working on #30 for this PR

Copy link
Contributor

@niansong1996 niansong1996 left a comment

Choose a reason for hiding this comment

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

Good work! Some questions:

  • Do you have to update the huggingface transformers library version?
  • On what device/GPU did you test the mathqa setting?

# for llama-based model
return output.lstrip().split("\n\n")[0].strip()
else:
return output.lstrip().split(tokenizer_eos_token)[0].split("\n\n")[0].strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

So LLAMA does not have an eos token?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yilunzhao
Copy link
Member Author

yilunzhao commented Apr 14, 2023

  • Do you have to update the huggingface transformers library version?

Yes. There has not been an official release that supports LLAMA, so I installed the transformer library from source.

pip install git+https://github.com/huggingface/transformers
  • On what device/GPU did you test the mathqa setting?

The experimental setting was recorded in this google doc. I will use this doc for updates.

@niansong1996
Copy link
Contributor

To reiterate the action items discussed in the meeting:

  • add the command to install the library so we can replicate the results

Also I was wondering if this PR is ready to merge? You also mentioned that there are some edge cases that haven't been handled?

@yilunzhao
Copy link
Member Author

  • add the command to install the library so we can replicate the results

I just updated the requirements.txt in the second commit.

Also I was wondering if this PR is ready to merge? You also mentioned that there are some edge cases that haven't been handled?

Yes. LLAMA, Alpaca, and santacoder should work fine using the config file: /home/lily/yl2465/code/NLP4Code/finetuning/training_configs/few_shot/mathqa-8_fixed_mathqa_shots_llama.yaml

@niansong1996
Copy link
Contributor

@yilunzhao Can you resolve the conflicts and also merge from main to run the CI tests?

@niansong1996
Copy link
Contributor

Great, merging this PR now

@niansong1996
Copy link
Contributor

@yilunzhao Check my comments on #46

Since we can't reopen a merged PR, can you submit a new PR and point it to this PR instead?

Let me know if you have any questions. Sorry about the confusion.

@yilunzhao
Copy link
Member Author

Hi @niansong1996, I have submit a new PR #48, could you please have a look at it?
This time, I add an instruction in README about how to install transformers library from source to run LLAMA-based models, rather than modifying the requirement.txt.

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.

2 participants