-
Notifications
You must be signed in to change notification settings - Fork 158
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
Add Mistral support to Bedrock #703
Conversation
36f066f
to
2c6bbc8
Compare
See this thread more more information: https://github.com/griptape-ai/griptape/issues/244 | ||
|
||
Returns: | ||
BedrockLlamaTokenizer: The tokenizer for this driver. |
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.
BedrockLlamaTokenizer
-> looks like a copy/paste error
if self._tokenizer: | ||
return self._tokenizer | ||
else: | ||
self._tokenizer = BedrockMistralTokenizer(model=self.prompt_driver.model) | ||
return self._tokenizer |
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.
Super nit:
if self._tokenizer is None:
self._tokenizer = BedrockMistralTokenizer(model=self.prompt_driver.model)
return self._tokenizer
Args: | ||
prompt_stack: The `PromptStack` to convert. | ||
""" | ||
system_input = next((i for i in prompt_stack.inputs if i.is_system()), None) |
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.
Why does this assume a max of one system input?
Should you raise an exception if this assumption violated (rather than ignoring silently)?
prompt_lines = [self.BOS_TOKEN] | ||
for prompt_input in non_system_inputs: | ||
if prompt_input.is_assistant(): | ||
prompt_lines.append(f"{prompt_input.content}{self.EOS_TOKEN} ") | ||
else: | ||
prompt_lines.append(f"[INST] {prompt_input.content} [/INST]") |
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.
Won't this end up with unbalanced BOS/EOS tokens? I didn't quickly get a great understanding of the mistrial docs on hugging face. Is unbalanced BOS/EOS tokens ok?
2c6bbc8
to
f9bf507
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
f9bf507
to
65d6085
Compare
Going to close this in favor of the work that will be done for this. Prompt Model Drivers may become a thing of the past. |
Closes #664