8000 gguf-py : add support for sub_type (in arrays) in GGUFWriter add_key_value method by CISC · Pull Request #13561 · ggml-org/llama.cpp · GitHub
[go: up one dir, main page]

Skip to content

gguf-py : add support for sub_type (in arrays) in GGUFWriter add_key_value method #13561

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

CISC
Copy link
Collaborator
@CISC CISC commented May 15, 2025

Without this it's not possible to pass on original metadata array sub-types, thus coercing them to whatever GGUFWriter decides based on the value type.

Fixes #13557

@CISC CISC requested a review from compilade May 15, 2025 10:06
@github-actions github-actions bot added the python python script changes label May 15, 2025
@CISC
Copy link
Collaborator Author
CISC commented May 17, 2025

@compilade gentle ping

@CISC
Copy link
Collaborator Author
CISC commented May 20, 2025

@compilade Did NorthSec go well? :)

@esrakorkmz
Copy link

failing checks
Python Type-Check / pyright type-check (pull_request)
Python Type-Check / pyright type-check (pull_request)Failing after 15s
Python Type-Check / pyright type-check (push)
Python Type-Check / pyright type-check (push)Failing after 14s

@CISC
Copy link
Collaborator Author
CISC commented May 22, 2025

failing checks

It's because I'm requiring a package that doesn't exist yet. :)

See CI on previous commits.

@CISC CISC requested a review from cebtenzzre May 24, 2025 20:38
@CISC
Copy link
Collaborator Author
CISC commented May 24, 2025

@cebtenzzre If you have time a review would be appreciated, seems @compilade is on vacation after NorthSec. :)

@compilade
Copy link
Collaborator

@compilade Did NorthSec go well? :)

@CISC It was fun, but surprisingly exhausting1, so I did take a break afterwards.

seems @compilade is on vacation after NorthSec. :)

Kind of :)
Sorry for the delays in my replies. I'm hopefully going back on track after this impromptu pause.

Footnotes

  1. Surprising because last year there was a challenge in which I've manually scanned hundreds (maybe thousands) of QR codes (before partially automating it), but this year it seems my endurance was lesser. Mostly attempted reverse-engineering challenges in this year's competition. Maybe that saturated (temporarily) some part of what I also use to review and reply to stuff ;)

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.

This seems good for arrays with a single sub-type (which in practice is the only kind of arrays).

If we ever need nested sub-types (e.g. for nested arrays), then it will probably be simple to extend the sub_type argument to be Sequence[GGUFValueType] | GGUFValueType | None (but that's not required for now).

I did not test running this (yet), but the changes seem good and I assume you tested by fixing #13557.

Comment on lines +1018 to +1019
if sub_type is not None:
ltype = sub_type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding some dynamic type-checking here could be useful (to check if all the elements of the array have the same type and are compatible with the given sub_type), but a traceback from this function isn't that useful because it's called when packing, not when adding the value to the writer.

Not sure what would be best here. I guess it's fine like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's very useful, it will mostly likely be used for passing existing GGUF arrays anyway.

We can always revisit if it becomes an issue.

@CISC
Copy link
Collaborator Author
CISC commented May 27, 2025

@compilade Did NorthSec go well? :)

@CISC It was fun, but surprisingly exhausting[1], so I did take a break afterwards.

Well earned, glad you had fun at least. :)

seems @compilade is on vacation after NorthSec. :)

Kind of :) Sorry for the delays in my replies. I'm hopefully going back on track after this impromptu pause.

No worries, welcome back!

@CISC
Copy link
Collaborator Author
CISC commented May 27, 2025

If we ever need nested sub-types (e.g. for nested arrays), then it will probably be simple to extend the sub_type argument to be Sequence[GGUFValueType] | GGUFValueType | None (but that's not required for now).

Sure, that will probably require a bit of work anyway. :)

I did not test running this (yet), but the changes seem good and I assume you tested by fixing #13557.

Yes, and some other GGUFs for good measure.

I will merge and tag later today.

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.

Misc. bug: gguf-new-metadata and gguf-editor-gui changes all integer arrays to INT32
3 participants
0