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

Made convert.py work with LLaMA 3 files distributed by meta #7568

Closed

Conversation

0xMana-git
Copy link

@0xMana-git 0xMana-git commented May 27, 2024

The fix is overall pretty hacky and not very clean, but it does work and doesn't have any pre vocab issues afaik
Improvements such as not using another script can be made, and detection for llama 3 is basically nonexistent, but I guess if someone just wants to get stuff working then this could be applicable

Simple math question test can be seen here(cpu inference, i messed up the cuda build):
image

@github-actions github-actions bot added the python python script changes label May 27, 2024
@teleprint-me
Copy link
Contributor

teleprint-me commented May 27, 2024

It should be noted that the convert.py script is in the process of being deprecated and will be moved to examples.

@mofosyne mofosyne added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label May 28, 2024
@0xMana-git
Copy link
Author

It should be noted that the convert.py script is in the process of being deprecated and will be moved to examples.

😭

@mofosyne
Copy link
Collaborator

Hmmm... as long as it doesn't break the legacy convert script let's get this merged in quickly before we do the refactor

@compilade
Copy link
Collaborator

compilade commented May 28, 2024

Hmmm... as long as it doesn't break the legacy convert script let's get this merged in quickly before we do the refactor

That's not a reason to avoid doing a proper review. This looks useful, but there are some parts of it I'd like to review in more detail.

Regarding the order of the merges, I think #7430 should be merged first, because it will be much easier to test this with the new file names and locations. (because things will break, especially since convert.py here assumes convert_llama_weights_to_hf.py is in the same directory)

@teleprint-me
Copy link
Contributor

teleprint-me commented May 28, 2024

@Manaball123 Do not lose hope. Programming isn't easy, but that's what makes it fun 😉.

@compilade If this is merged, get this merged this first. This should be merged before PR #7430. I'm going to be awhile. I did crack the vocab though. 🥲

My only issue with this PR is that the vocab wasn't properly handled. Other than that, I think it's a good start. It just needs some guidance.

@0xMana-git
Copy link
Author

You could probably merge convert_llama_weights_to_hf.py into the same file as-is, I didn't do it because it would make convert.py more cluttered than it already is lol
Regarding the vocab issue, can you elaborate on what it is? I could probably write a fix if it's nothing major.

@mofosyne
Copy link
Collaborator

mofosyne commented May 30, 2024

Heads up that #7430 has merged, so will need to refactor this pr

@0xMana-git
Copy link
Author

uh oh i think i messed something up :(

@compilade
Copy link
Collaborator

uh oh i think i messed something up :(

To fix this, check your git reflog and git reset to a previous commit (but read about --soft, --mixed, --hard, and --keep first (git reset --help)), then git push --force-with-lease this branch back to that commit.

@0xMana-git
Copy link
Author

Sorry for the inconvenience, made new pr here
😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python python script changes Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants