@phiharri looks good, is the sentinel value not required, I saw that in your original PR.

Not sure where I got that idea from tbh, pretty sure it isn't needed. Thought about checking len(key)<128 for potential overflow but it seems ctypes takes care of that already 🤷

>>> k='a'*130
>>> kvo.key=k.encode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: bytes too long (130, maximum length 128)

I think it is necessary to add an additional key and set it to the null byte, see https://github.com/ggerganov/llama.cpp/blob/6f9939d119b2d004c264952eb510bd106455531e/llama.cpp#L2250

Sure, how's this look? Elements get initialised empty/null but if you wanted to be explicit we can add
self._kv_overrides_array[-1].key = b'\0'

@phiharri that's correct however I think(?) we still need to store the reference to that byte string because we're passing it into a c data structure and want to avoid any memory issues.

Aren't we just modifying data in the existing _kv_overrides_array, not allocating new memory (unless doing create_string_buffer)?

Definitely seems the array gets null initialised so I don't think self._kv_overrides_array[-1].key = b'\0' is needed. https://docs.python.org/3/library/ctypes.html#arrays "the array contents is initialized to zeros"

Peeking the memory of the last element without doing any b'\0' assignment shows all zeros:

Doh you're right, I thought the key was a char * not a fixed size array, yes as long as it's zero initialized (I'd like to be explicit about this) then it should be okay.

Awesome, okay good to merge it then. Thanks for the contribution @phiharri