Skip to content

Allow override when adding value to ggufwriter #14194

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 1 commit into from
Jun 16, 2025

Conversation

huydt84
Copy link
Collaborator

@huydt84 huydt84 commented Jun 15, 2025

Discussion: #14164

This PR use a simple way to allow overriding value when add key-value to GGUF, which has been mentioned here: #14164 (comment)

This can cause drastic changes in many unexpected aspects, so please review carefully

@github-actions github-actions bot added the python python script changes label Jun 15, 2025
@huydt84 huydt84 requested review from CISC and ggerganov June 15, 2025 12:28
@ericcurtin ericcurtin requested a review from Copilot June 15, 2025 13:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the behavior of the add_key_value method in gguf_writer.py to allow overriding an existing key instead of raising an exception.

  • The exception for a duplicate key is replaced with a logger warning.
  • The existing key in the first dictionary of kv_data is unconditionally overwritten.
Comments suppressed due to low confidence (1)

gguf-py/gguf/gguf_writer.py:274

  • The change always updates the key in self.kv_data[0], which may not reflect the intended behavior if a duplicate key exists in a different dictionary within kv_data. Consider verifying that updating only at index 0 is correct, or update the key in its original location.
logger.warning(f'Duplicated key name {key!r}, overwriting it with new value {val!r} of type {vtype.name}')

Copy link
Collaborator

@CISC CISC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be ok, it's only a potential problem if you did something wrong when adding a new model, but will usually be caught later on if you failed to notice the warning.

Either way, adding some more people in to review for additional opinions. :)

@CISC CISC requested review from cebtenzzre and compilade June 15, 2025 17:24
@ngxson
Copy link
Collaborator

ngxson commented Jun 15, 2025

I would admit that I myself sometimes struggle with the fact that we don't allow overwriting KV metadata.

The cleaner solution is to refactor the conversion code inside convert_hf_to_gguf.py to avoid adding the same key twice, but it proves to be quite complicated in some cases. For example, overwriting chat template is controlled by set_vocab which is quite complicated to modify.

So yes I think this can be a valid solution for now.

Copy link
Collaborator

@compilade compilade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error was originally added in #7827, mainly because duplicate keys often were mostly accidental.

It's also because before that PR, it wasn't really possible to override an existing key-value pair, and so it really was added twice to the GGUF. (that's no longer the case, and overriding the value of a key should do what you expect)

I guess it can make sense to allow it (while still warning).

@CISC CISC merged commit 4ad2436 into ggml-org:master Jun 16, 2025
4 checks passed
@huydt84 huydt84 mentioned this pull request Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python python script changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants