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

Fix/errors for some models including Mistral-7b and Flan-ul2 #388

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

lachlancahill
Copy link
Contributor

  • Use os.path.basename for all OS compatibility including Windows.
  • Identify if the hub has downloaded to a snapshots subfolder and move to root directory before committing so that commits work.

Lachlan Cahill added 2 commits November 19, 2023 11:43
…ub the binary files write to a snapshots/<uuid> subdirectory. Then when the files are committed, they cannot be found and the commit fails with an error.
@Narsil
Copy link
Collaborator

Narsil commented Nov 20, 2023

Thanks a lot for your contribution @lachlancahill .

Happy to include the Windows fix (my bad on that path splitting).

For the snapshots thing I don't really like it as it is (anyone could have used any path there).
Are you ok, if we make it another PR and you explain a little bit more what's happening for you here ?
What's happening and what is failing ? So we can maybe find a workaround that is a little bit more robust.

bindings/python/convert.py Outdated Show resolved Hide resolved
@lachlancahill
Copy link
Contributor Author

Thanks a lot for your contribution @lachlancahill .

Happy to include the Windows fix (my bad on that path splitting).

For the snapshots thing I don't really like it as it is (anyone could have used any path there). Are you ok, if we make it another PR and you explain a little bit more what's happening for you here ? What's happening and what is failing ? So we can maybe find a workaround that is a little bit more robust.

My pleasure, thank you for your great feedback.

I've removed the 'snapshots' change from the PR though worth keeping in mind that if we take out that whole block we need to add back the original local_filenames.append(sf_filename) line. I've actioned this in the latest commit.

To provide more detail on the snapshots issue, while investigating errors I noticed the folder structure for Mistral (similar for flan-ul2) was like the below in the temporary directory:

|-- modelss--mistralai--Mistral-7B-v0.1
    |-- .locks
        |-- models--mistralai--Mistral-7B-v0.1
    |-- model.safetensors.index.json
    |-- models--mistralai--Mistral-7B-v0.1
        |-- blobs
        |-- refs
            |-- main
        |-- snapshots
            |-- 5e9c98b96d071dce59368012254c55b0ec6f8658
                |-- model-00001-of-00002.safetensors
                |-- model-00002-of-00002.safetensors
                |-- pytorch_model-00001-of-00002.bin
                |-- pytorch_model-00002-of-00002.bin
                |-- pytorch_model.bin.index.json

I think the version I was working with had a step which checked the converted files, and this non-standard folder structure was causing issues (I think the checking function was looking in the root folder for the .safetensors files). In the latest version it looks like you have removed that check which I think makes this change unnecessary. Thanks for all your great work, this is such a valuable resource. 😄

@Narsil
Copy link
Collaborator

Narsil commented Nov 27, 2023

Yes I removed the check because we're now pretty confident about the conversions as they exist, and removing it allows to convert models with much less RAM than the actual model requires.

I would keep it if I knew a given machine had enough RAM to run the check, but it seems too involved/unreliable atm.

@Narsil Narsil merged commit 9c81742 into huggingface:main Nov 27, 2023
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