-
Notifications
You must be signed in to change notification settings - Fork 364
Adding support for MiniMax01Text #38
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
Adding support for MiniMax01Text #38
Conversation
|
|
model used here is a untrained one because the only original is 400B params Goekdeniz-Guelmez/MiniMax01Text-Dev |
|
|
Is this PR ready for review? Do you need help testing the actual model? |
|
This PR is ready to merge, but to be 100% sure it would be great to test out the actual model. |
|
I tried running the model. It still has some issues. First off, the tokenizer chat template requires a different input format. So we'll need to figure out how to deal with that. It crashes if you do what we have now: Instead it wants: When using the right spec for that, it then crashes with the following: |
|
Somehow it's predicting a token that is not in the token map: |
|
The vocab size is 200064 (see here, so it looks like it could be an issue with making the tokenmap 🤔 |
|
Well there's definitely nothing higher than 200026 in the tokenizer so I don't know what's up with that vocab size mismatch. I think it's a bit of a red herring though as the model probably has a bug causing it to generate a bad token id. To debug this one needs to use a machine with >=256Gb. I can help out soon if needed lmk. |
|
Thanks for testing it out! That's interesting, I have this HF this is literally the same model architecture, but a smaller param model untrained, and this was the one I was developing with and it works, but yea well need the OG model. As for the machine thanks a lot for your help, however I would check with @ivanfioravanti for his machine, I'll try to continue finding the error after two weeks. |
|
@Goekdeniz-Guelmez @awni Thanks for your work! How's the PR going? |
|
Hello @sriting, I will be continuing to work on it on friday :D. |
|
Can you guys try again? should be fixed now. |
|
Ok I will try it. But since this is a one-off thing with the MiniMax chat template .. I would prefer to update their chat template in the mlx-community version to be consistent with what every other model expects rather than changing the way we build messages in |
|
Exactly, we're on the same wavelength! My small dev model has that already, so when a quantized version gets uploaded I'll PR the working chat template, as for this, lets get the inference first working and I'll rebase the tokenizer_utils back to the original. I can then create a issue here outlining this problem with the OG for the users. |
|
I'm not getting good results. It starts out with gibberish then it crashes with a missing token error because the model is producing a token outside the vocab. That's kind of a bug that we crash for that.. but it also usually means the implementation is off. |
|
Thanks for trying it again! Most likely my implementation is wrong, I'll look into it later today. |
|
I am getting the same gibberish words 😂 |
|
@awni @Goekdeniz-Guelmez |
|
@KartavyaBagga nope, haven’t worked on it since the last push, but haven forgotten it. Some other research and mlx stuff has a higher priority then that, but eventually it’ll merge. |
|
Hey @awni would you mind trying it out again, did quite a few changes in the code, also used a tiny model trained on "Hello World!" again, and it looks like it works: |
|
@awni will be closing this since we dont need it anymore. |
|
Yes makes sense. I am not sure if anyone will want to run the old minimax text 01, if so we can revive this / find a way to salvage the relevant pieces. Thanks for working on it, sorry for the delay getting it reviewed. |
|
the minimax 1 and 2 are the same mixtral type so both should work :D |
No description provided.