-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
base: master
Are you sure you want to change the base?
Conversation
@compilade gentle ping |
@compilade Did NorthSec go well? :) |
failing checks |
It's because I'm requiring a package that doesn't exist yet. :) See CI on previous commits. |
@cebtenzzre If you have time a review would be appreciated, seems @compilade is on vacation after NorthSec. :) |
@CISC It was fun, but surprisingly exhausting1, so I did take a break afterwards.
Kind of :) Footnotes
|
There was a problem hiding this 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.
if sub_type is not None: | ||
ltype = sub_type |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Well earned, glad you had fun at least. :)
No worries, welcome back! |
Sure, that will probably require a bit of work anyway. :)
Yes, and some other GGUFs for good measure. I will merge and tag later today. |
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