-
Notifications
You must be signed in to change notification settings - Fork 83
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
ui.Chat()
now correctly handles new ollama.chat()
return value introduced in ollama 0.4
#1787
Changes from 6 commits
ee260c5
97e24a6
9efe068
b26a435
14539a8
29ab06a
2e39d53
0ae6c71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,12 +45,23 @@ def encode( | |
TokenEncoding = Union[TiktokenEncoding, TokenizersTokenizer] | ||
|
||
|
||
def get_default_tokenizer() -> TokenizersTokenizer | None: | ||
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( | ||
cpsievert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it's opt-in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
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.
Is some of the context here that you have a message normalizer for Pydantic models?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)