Conversation
Detect obsolete options and warn if they are detected, then strip them from the request. Unknown options still pass through. Adds min_p and draft_num_predict. Fixes ollama/ollama#18480
ruff 0.16 started formatting Python code blocks in Markdown, so the unpinned `hatch fmt --check` in CI now fails on the untouched README. Keep the README examples hand-wrapped.
pdevine
left a comment
pdevine
left a comment
There was a problem hiding this comment.
Looks fine to me. You should double check with @ParthSareen
| top_k: Optional[int] = None | ||
| top_p: Optional[float] = None | ||
| tfs_z: Optional[float] = None | ||
| typical_p: Optional[float] = None |
There was a problem hiding this comment.
Could we keep these fields around and just leave them out of requests? Existing code that reads options.typical_p now crashes, even though passing it into Options still works with a warning. A test that checks reading the old fields would help here.
| if not isinstance(options, Mapping): | ||
| return options |
There was a problem hiding this comment.
This still lets typical_p through when someone uses Options().model_copy(update={'typical_p': 0.5}). I checked the request body and it’s sent without a warning, so the server would still reject it. Could we strip these options before sending the request and add a test for this case?
|
I'm not sure if we really want the SDKs to modify the requests - it seems like that logic should belong to Ollama itself so it is consistent. For example, if we want to allow GGUFs to continue to support typical_p in API requests that would probably be the friendliest way of deprecation or just strip there if that's what we want. But the SDKs should mark the field as deprecated in their docs and/or warn since otherwise many people will never know. |
4 participants
Detect obsolete options and warn if they are detected, then strip them from the request. Unknown options still pass through. Adds min_p and draft_num_predict.
Fixes ollama/ollama#18480