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

The readme does not work #27

Closed
pevnak opened this issue Feb 14, 2025 · 6 comments
Closed

The readme does not work #27

pevnak opened this issue Feb 14, 2025 · 6 comments

Comments

@pevnak
Copy link

pevnak commented Feb 14, 2025

The readme does not work, since tokenizer_from_file was renamed to HuggingFaceTokenizers.from_file I guess.

@pevnak
Copy link
Author

pevnak commented Feb 14, 2025

There seems to be more bugs.
I have tried to update it to this

using JSON3, HuggingFaceTokenizers, Jjama3
const HFT =  HuggingFaceTokenizers


function Jjama3.nexttoken!(tokens, model, sampler, logits, tokenizer_for_printing)
    tokens[model.pos+1] = sampler(logits[:, end, 1])
    !isnothing(tokenizer_for_printing) && print(HFT.decode(tokenizer_for_printing, [tokens[model.pos+1]], skip_special_tokens = false))
end

config = JSON3.read(read("SmolLM2-360M-Instruct/config.json", String))
model = load_llama3_from_safetensors("SmolLM2-360M-Instruct/model.safetensors", config)
tkn = HFT.from_file(Tokenizer, "SmolLM2-360M-Instruct/tokenizer.json")

prompt = HFT.encode(tkn, "Tell me the two worst things about Python.")
generate(model, prompt.ids,
        max_new_tokens=500,
        tokenizer_for_printing=tkn,
        end_token = HFT.encode(tkn, "<|im_end|>")[end]);

But it does generates garble. Are my fixes correct?

@murrellb
Copy link
Member

Thanks! Should be fixed now on main (and the readme example gives sensible output - on my machine at least).

The reason your changes didn't work is because Jjama3 needs to do a little translation from the HuggingFaceTokenizer (mostly for 1-indexing).

@murrellb
Copy link
Member

Spoke slightly too soon - you also need to import HuggingFaceTokenizers to avoid a name clash.

@murrellb
Copy link
Member

...which should now be fixed in the readme on main.

@murrellb
Copy link
Member

Image

@pevnak
Copy link
Author

pevnak commented Feb 15, 2025

Thanks for help. It seems to work now.

@pevnak pevnak closed this as completed Feb 15, 2025
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

No branches or pull requests

2 participants