ui.Chat() now correctly handles new ollama.chat() return value introduced in ollama 0.4
#1787
Conversation
shiny/ui/_chat_tokenizer.py
Outdated
| def get_default_tokenizer() -> TokenizersTokenizer: | ||
| try: | ||
| from tokenizers import Tokenizer | ||
|
|
||
| return Tokenizer.from_pretrained("bert-base-cased") # type: ignore | ||
| except Exception: | ||
| pass | ||
|
|
||
| return None | ||
| except ImportError: | ||
| raise ValueError( | ||
| "Failed to download a default tokenizer. " | ||
| "A tokenizer is required to impose `token_limits` on `chat.messages()`. " | ||
| "To get a generic default tokenizer, install the `tokenizers` " | ||
| "package (`pip install tokenizers`). " | ||
| ) | ||
| except Exception as e: | ||
| raise ValueError( | ||
| "Failed to download a default tokenizer. " | ||
| "A tokenizer is required to impose `token_limits` on `chat.messages()`. " | ||
| "Try downloading a different tokenizer using " | ||
| "`tokenizers.Tokenizer.from_pretrained()`. " | ||
| f"Error: {e}" | ||
| ) from e |
There was a problem hiding this comment.
This change is orthogonal to the main fix of this PR, but it's a good idea, and I ran into because bert-base-cased temporarily went offline for a couple of the test runs.
There was a problem hiding this comment.
Are token_limits always imposed? Or is that opt-in or opt-out? It's hard to follow the thread back to where that would be chosen by the user and you might want to include that information in the message, e.g. if it's possible to disable token limits.
There was a problem hiding this comment.
Seems like the second message is missing some instruction about how to choose a specific tokenizer that was in the original message. I also think it'd be nice to include how to turn off the need for a tokenizer, but that might be obvious from the code you'd write to get here.
| if isinstance(ChatResponse, dict): | ||
| return "message" in message and super().can_normalize( | ||
| message["message"] | ||
| ) | ||
| else: | ||
| return isinstance(message, ChatResponse) |
There was a problem hiding this comment.
Is some of the context here that you have a message normalizer for Pydantic models?
There was a problem hiding this comment.
I don't think you necessarily need to leave a comment, but it'd be helpful for my understanding of the code if you just quickly explained how this fixes the problem (beyond the simple explanation that before it was a dict and now it isn't, that part I get).
There was a problem hiding this comment.
DictNormalizer works for either case since ollama defines __getitem__() on the pydantic model. I suppose that is a weird/subtle thing that requires extra context, and it'd be nice to take advantage of stronger pydantic typing, but I opted for the minimal change (especially if we're going to support older versions)
Ollama 0.4 changed the return type of
ollama.chat()(ChatResponse) from aTypedDictto apydantic.BaseModel. As a result, passing that return value directly to.append_message()or.append_message_stream()doesn't work:This PR fixes the issue, adds a unit test, and fixes another failing unit test.