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

convert : get general.name from model dir, not its parent #5615

Merged
merged 2 commits into from
May 16, 2024

Conversation

cebtenzzre
Copy link
Collaborator

When I converted dl/Open-Orca_Mistral-7b-OpenOrca, convert.py would set general.name to dl. Now it sets it to Open-Orca_Mistral-7b-OpenOrca like convert-hf-to-gguf.py would.

Am I missing something? I don't understand if the previous behavior was intentional. TheBloke's models on HF don't seem to be affected by this... cc @TheBloke in case you have an opinion?

@slaren
Copy link
Collaborator

slaren commented Feb 20, 2024

Originally this is from 8f8c28e. My guess is that it was done this way due to the directory structure of the original llama-2 distribution files.

@ggerganov
Copy link
Owner

My guess is that it was done this way due to the directory structure of the original llama-2 distribution files.

Yes, that was the intention. It's not the smartest logic, so probably it can be improved. But I like to think that convert.py's main purpose is to convert the original models released by Meta. So it should be compatible in some way or another with the directory structure of those models

@cebtenzzre
Copy link
Collaborator Author

cebtenzzre commented Feb 20, 2024

My guess is that it was done this way due to the directory structure of the original llama-2 distribution files.

Where do I get these? I know about https://github.com/facebookresearch/llama but their download.sh just downloads files to a folder with a name like "llama-2-7b" (MODEL_PATH) in the current directory (TARGET_FOLDER of ".").

@cebtenzzre
Copy link
Collaborator Author

Could it be clearly explained how the expected folder structure is meant to be created, so that I can verify that convert.py sets general.name correctly in all cases? So far it seems to me that some manual operations are required to result in the name of the parent directory being relevant, Meta checkpoints or otherwise.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

I have this on my machine:

$ tree llama*
llama
├── 13B
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   └── params.json
├── 30B
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   ├── consolidated.02.pth
│   ├── consolidated.03.pth
│   └── params.json
├── 65B
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   ├── consolidated.02.pth
│   ├── consolidated.03.pth
│   ├── consolidated.04.pth
│   ├── consolidated.05.pth
│   ├── consolidated.06.pth
│   ├── consolidated.07.pth
│   └── params.json
├── 7B
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   └── params.json
├── tokenizer.model
└── tokenizer_checklist.chk
llama2
├── CODE_OF_CONDUCT.md
├── CONTRIBUTING.md
├── LICENSE
├── MODEL_CARD.md
├── README.md
├── Responsible-Use-Guide.pdf
├── USE_POLICY.md
├── download.sh
├── example_chat_completion.py
├── example_text_completion.py
├── llama
│   ├── __init__.py
│   ├── generation.py
│   ├── model.py
│   └── tokenizer.py
├── llama-2-13b
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   └── params.json
├── llama-2-13b-chat
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   └── params.json
├── llama-2-70b
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   ├── consolidated.02.pth
│   ├── consolidated.03.pth
│   ├── consolidated.04.pth
│   ├── consolidated.05.pth
│   ├── consolidated.06.pth
│   ├── consolidated.07.pth
│   └── params.json
├── llama-2-70b-chat
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   ├── consolidated.01.pth
│   ├── consolidated.02.pth
│   ├── consolidated.03.pth
│   ├── consolidated.04.pth
│   ├── consolidated.05.pth
│   ├── consolidated.06.pth
│   ├── consolidated.07.pth
│   └── params.json
├── llama-2-7b
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   └── params.json
├── llama-2-7b-chat
│   ├── checklist.chk
│   ├── consolidated.00.pth
│   └── params.json
├── requirements.txt
├── setup.py
├── tokenizer.model
└── tokenizer_checklist.chk

I've forgotten how I downloaded the llama models, but the llama2 were downloaded through the download.sh script.

The v2 support is more important to get right and the v1 is nice to have, but can be ignored as it's quite outdated now. Apologies for this PR getting accidentally ignored - I think the change is good

@mofosyne mofosyne self-assigned this May 10, 2024
@mofosyne mofosyne added refactoring Refactoring Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels May 10, 2024
@mofosyne mofosyne added the merge ready indicates that this may be ready to merge soon and is just holding out in case of objections label May 14, 2024
@mofosyne
Copy link
Collaborator

mofosyne commented May 14, 2024

I'm pretty sure it's safe to merge as I was trying to do the same a while ago as well... and gg thinks it's safe to do so based on his directory structure.

Will merge soon unless someone else can see any issue.

(Context: Was going though list of all approved but not yet merged https://github.com/ggerganov/llama.cpp/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved PRs. Do have a check.)

@mofosyne mofosyne merged commit dda64fc into master May 16, 2024
25 checks passed
@mofosyne mofosyne removed the merge ready indicates that this may be ready to merge soon and is just holding out in case of objections label May 16, 2024
teleprint-me pushed a commit to teleprint-me/llama.cpp that referenced this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring 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.

None yet

4 participants