-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Add Anthropic invocation layer #4818
Conversation
# if a List[str] is used we must also set is_pretokenized to True. | ||
# We split at spaces because if we pass the string directly the encoded prompts | ||
# contains strange characters in place of spaces. | ||
encoded_prompt: Encoding = self.tokenizer.encode(prompt.split(" "), is_pretokenized=True) |
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.
The code comment and the code seem to be out of sync? Do we deal with List[str] here, most likely not, right?
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.
No, the comment is correct.
At line 146 we fail when prompt
is a List
so in here it can only be a str
.
The first two lines clarify why we're setting is_pretokenized
and the other clarifies why we split prompt
.
|
||
kwargs_with_defaults = self.model_input_kwargs | ||
|
||
if "stop_sequence" in kwargs: |
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.
We have stop_words as the parameter at the prompt node init level that we then pass to the invocation layer. And this invocation layer then translates "stop_words" to the specific parameter name in the invocation layer. As we stand here, but I am not 100% sure, it seems like stop _words from PromptNode level will not get translated to "stop_sequence" parameter. Please double check
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.
Update: it will get translated but double-check why the default case when no stop_words are passed from prompt node level text generation fails
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.
The test_invoke_non_streamed
integration test passes with not issues.
That one invokes the layers with just the prompt, it should be enough shouldn't it?
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.
It should, but in this case, it is a problem because stop_words from PromptNode get passed as None because we allow None. And Anthropic, unlike all other providers, doesn't like None and barfs the exception.
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.
Ah, makes sense. I'll see to handle that case.
stream = ( | ||
kwargs_with_defaults.get("stream", False) or kwargs_with_defaults.get("stream_handler", None) is not None | ||
) | ||
stop_words = kwargs_with_defaults.get("stop_words", [human_prompt]) |
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 believe we should ensure that user-specified stop_words should be added to the default Anthropic uses, i.e. we should have a list that contains user-specified stop_words and their default ["\n\nHuman:"]
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 think it's added in any case by Anthropic server side but I handled it in any case.
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.
Left some minor comments regarding stop_words
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.
LGTM
Proposed Changes:
Add new invocation layer to support Anthropic Claude.
How did you test it?
I wrote tests and run them locally.
Notes for the reviewer
Supersedes #4512.