Reformat models for the rope parameter args#953
Conversation
…tialize rope_theta
…from rope_parameters
…with post_init handling
…with post_init handling
…with post_init handling
|
@angeloskath since im already on the house keeping, should I make the head dim argument on all models optional too? Maybe make the classes have correct input/output type definitions etc. mlx-lm-lora and other would benefit from it imo. |
|
Thanks @Goekdeniz-Guelmez that looks like a great start. We can migrate the head dim as well but let's finish the rope parameters migration. I would make sure that if a head dim is optional the model uses it correctly eg |
|
@angeloskath agreed! Let's first get rope params merged. Should be mergeable now |
|
@Goekdeniz-Guelmez removing the defaults tends to break things 😉 Running the tests would save us a round-trip |
…s including rope_theta
|
@angeloskath Done! |
|
Nice, sorry for the back and forth from my side now but shouldn't the other rope parameters also be moved from |
|
no worries! yes, that would make thinks easier |
- Increased default value of `rope_theta` from 10000.0 to 100000.0 in various model argument classes. - Introduced `rope_scaling` and `rope_traditional` parameters in several models to enhance flexibility in rope parameter configurations. - Ensured consistent handling of `rope_parameters` across models including `deepseek`, `exaone`, `gemma`, `glm`, `hunyuan`, `llama`, `qwen`, and others.
|
@angeloskath shoudl have all the params in and tests passed too. |
|
will be added the new models and then its mergeable @angeloskath |
|
How does it compare to #1465 ? |
|
the existing model configs use the old version where the rope parameters where defined separately, the new one has them under the "rope_parameters" so every model config needs that update, #1465 this just fixes the registration definition of the tokeniser object itself |
3 participants
for the transformers v5