Skip to content

force int cast #1170

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

Merged
merged 3 commits into from
Apr 25, 2023
Merged

force int cast #1170

merged 3 commits into from
Apr 25, 2023

Conversation

ostix360
Copy link
Contributor

.0 in the config file for the lora_alpha param
and I got this error

fout.write(struct.pack("ii", int(params["r"]), params["lora_alpha"]))
struct.error: required argument is not an integer

I just cast

 .0 in the config file for the lora_alpha param
@prusnak
Copy link
Collaborator

prusnak commented Apr 25, 2023

Shouldn't we fix the config file instead rather than silencing this bug?

@ostix360
Copy link
Contributor Author

If I have this error with ist downloading the Lora config from Hugging face so I think it's not the only one to have a .0 for the param's Lora-alpha
And the patch cost nothing in terms of performance

@slaren
Copy link
Member

slaren commented Apr 25, 2023

The mistake here seems to be assuming that lora_alpha is an integer, if it is actually a float. In that case, it should be exported as a float as well. At the very least, we need to check that int(lora_alpha) == lora_alpha.

@ostix360
Copy link
Contributor Author

Indeed lora_aplha can be a float value but if this equality int(lora_alpha) == lora_alpha is not true what are supposed to write in the file?
In the major case alpha Lora is an integer but if it's not and we write it as a float value on the ggml file, does it change something when it injected to the the model we run it in inference?

@prusnak
Copy link
Collaborator

prusnak commented Apr 25, 2023

The documentation here tells that lora_alpha is an int: https://opendelta.readthedocs.io/en/latest/modules/deltas.html

I think there are two ways how to proceed:

  1. use this PR, but add assert int(lora_alpha) == lora_alpha to the conversion script
  2. change lora_alpha from int to double in both convert-lora-to-ggml.py and llama.cpp

Option 2 has two subvariants:
a. do the change and do not bump the version
b. do the change and bump the version

Which way do you want to go @slaren? 1, 2a or 2b?

@slaren
Copy link
Member

slaren commented Apr 25, 2023

If the documentation says that it is an int, just go with option 1 as a sanity check and be done with it.

@prusnak
Copy link
Collaborator

prusnak commented Apr 25, 2023

Note to self: don't try to write valid code as a GitHub suggestion on a phone 😅

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.

3 participants