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

Move convert.py to examples/convert-legacy-llama.py #7430

Merged
merged 13 commits into from
May 30, 2024

Conversation

Galunid
Copy link
Collaborator

@Galunid Galunid commented May 21, 2024

There's little use for this one, and a lot of confusion.

@github-actions github-actions bot added examples python python script changes labels May 21, 2024
@ggerganov
Copy link
Owner

We should update readme instructions, ggml-ci and anything else that references the script. "GGUF My Repo" would also need an update: https://huggingface.co/spaces/ggml-org/gguf-my-repo/blob/main/app.py#L29

@Galunid Galunid marked this pull request as draft May 21, 2024 14:18
@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 21, 2024
@mofosyne
Copy link
Collaborator

No torch?

My understanding is that it's for safetensors

@@ -17,7 +17,7 @@ Also, it is important to check that the examples and main ggml backends (CUDA, M
### 1. Convert the model to GGUF

This step is done in python with a `convert` script using the [gguf](https://pypi.org/project/gguf/) library.
Depending on the model architecture, you can use either [convert.py](../convert.py) or [convert-hf-to-gguf.py](../convert-hf-to-gguf.py).
Depending on the model architecture, you can use either [convert-hf-to-gguf.py](../convert-hf-to-gguf.py) or [examples/convert-no-torch.py](../examples/convert-no-torch.py) (for `llama/llama2` models in `.pth` format).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that .pth stands for pytorch, I think this name could be misleading. Why not call it convert-llama.py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

convert-legacy-llama.py maybe? I'd like to avoid giving impression that it can be used for converting llama 3

@Galunid
Copy link
Collaborator Author

Galunid commented May 21, 2024

No torch?

My understanding is that it's for safetensors

No torch as in no torch dependency, though I can see why it'd be confusing. I'll try to come up with something better. Suggestions welcome ;)

@Galunid
Copy link
Collaborator Author

Galunid commented May 21, 2024

Relevant huggingface space PR https://huggingface.co/spaces/ggml-org/gguf-my-repo/discussions/68

@teleprint-me
Copy link
Contributor

teleprint-me commented May 21, 2024

I always just named it convert-torch.py because that's what it did. I mean, if you really want to be clear about it, convert-torch-to-gguf.py. The issue is that it supports safetensors as well and python 3.12 breaks the CRC compatibility with the torch conversion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

convert-hf-to-gguf.py imports should be changed to reflect the rename

from convert import LlamaHfVocab

Copy link
Contributor

@teleprint-me teleprint-me May 21, 2024

Choose a reason for hiding this comment

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

It should be moved to vocab.py. Anything related to the vocab that's still needed.

@github-actions github-actions bot added documentation Improvements or additions to documentation build Compilation issues script Script related devops improvements to build systems and github actions labels May 21, 2024
@mofosyne
Copy link
Collaborator

Hmmmm... Do we have a conversion reference table so people can identify what file type to use with each script?

If not for code repo docs maybe that's a wiki entry

@mofosyne
Copy link
Collaborator

Not against this idea as convert.py is indeed a bit confusing, but this PR is still appearing broken with the CI.

I do wonder if we could somehow consolidate all these into one convert.py... and have convert.py call specific convert-*.py based on detection of the model source etc...

@mofosyne
Copy link
Collaborator

If you decide to go though the consolidation approach, see if you can also consolidate some aspect like how we generate default naming convention gguf.

This will make it easier to update it as the standards evolve.

gguf-py/gguf/vocab.py Outdated Show resolved Hide resolved
@teleprint-me
Copy link
Contributor

teleprint-me commented May 24, 2024

Vocab factory is missing too. Not sure how to handle that yet.

@Galunid
Copy link
Collaborator Author

Galunid commented May 25, 2024

Vocab factory is missing too. Not sure how to handle that yet.

I don't think there's any use for vocab factory in convert-hf-to-gguf.py, nor any other scripts. I think it's fine to leave it in convert-llama-legacy.py since that's the only place we can expect to use it.

@Galunid Galunid marked this pull request as ready for review May 26, 2024 17:57
@Galunid Galunid changed the title Move convert.py to examples/convert-no-torch.py Move convert.py to examples/convert-legacy-llama.py May 27, 2024
Copy link
Collaborator

@compilade compilade left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I think https://huggingface.co/spaces/ggml-org/gguf-my-repo/discussions/68 should be merged first to avoid breaking that HF space.

@ggerganov, @phymbert, @Vaibhavs10

@teleprint-me
Copy link
Contributor

Awesome! I've been waiting for this to be merged so I can integrate this into my PR. This will be incredibly useful so I don't need to duplicate or reinvent APIs.

Copy link
Collaborator

@mofosyne mofosyne left a comment

Choose a reason for hiding this comment

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

Hoping this wont break my PR #7499 (review)

Looks like a good idea and good to refactor at this stage. Let's merge in once the hugging face PR gets merged in.


But let's merge if hf have not merged theirs in still by the time the main branch is stable.

@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 28, 2024
@Vaibhavs10
Copy link
Collaborator

Heya! @mofosyne, @Galunid and @ggerganov - I just merged a PR (https://huggingface.co/spaces/ggml-org/gguf-my-repo/discussions/73) to default to the convert-hf script.

Good from my side to merge this!

Copy link
Collaborator

@Vaibhavs10 Vaibhavs10 left a comment

Choose a reason for hiding this comment

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

Also, thanks a ton for taking this up and opening the PR! 🤗

Copy link
Collaborator

@mofosyne mofosyne left a comment

Choose a reason for hiding this comment

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

Well, it's now or never. Let's see how many convert.py PRs we have to fix, but looking forward to a cleaner codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compilation issues devops improvements to build systems and github actions documentation Improvements or additions to documentation examples merge ready indicates that this may be ready to merge soon and is just holding out in case of objections 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 script Script related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants