Skip to content
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 Cohere PromptNode invocation layer #4827

Merged
merged 16 commits into from
May 12, 2023
Merged

feat: Add Cohere PromptNode invocation layer #4827

merged 16 commits into from
May 12, 2023

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented May 7, 2023

Related Issues

Proposed Changes:

Add Cohere command and command-light model support. Both models support token streaming

How did you test it?

Manually, unit tests

Notes for the reviewer

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@vblagoje vblagoje requested a review from a team as a code owner May 7, 2023 10:02
@vblagoje vblagoje requested review from bogdankostic and removed request for a team May 7, 2023 10:02
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels May 7, 2023
@coveralls
Copy link
Collaborator

coveralls commented May 7, 2023

Coverage Status

Coverage: 37.72%. Remained the same when pulling da90aa7 on cohere into d322bee on main.

Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking already pretty good, I just found some minor things and have a few questions.

haystack/errors.py Outdated Show resolved Hide resolved
super().__init__(message=message, status_code=429, send_message_in_event=send_message_in_event)


class CohereInferenceUnauthorizedError(HuggingFaceInferenceError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, CohereInferenceError should probably be the base class.

haystack/nodes/prompt/invocation_layer/cohere.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/cohere.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/cohere.py Outdated Show resolved Hide resolved
test/prompt/invocation_layer/test_cohere.py Show resolved Hide resolved
test/prompt/invocation_layer/test_cohere.py Outdated Show resolved Hide resolved
test/prompt/invocation_layer/test_cohere.py Outdated Show resolved Hide resolved
test/prompt/invocation_layer/test_cohere.py Outdated Show resolved Hide resolved
Comment on lines 167 to 172
def test_supports():
"""
Test that supports returns True correctly for CohereInvocationLayer
"""
# doesn't support fake model
assert not CohereInvocationLayer.supports("fake_model", api_key="fake_key")

# supports cohere command with api_key
assert CohereInvocationLayer.supports("command", api_key="fake_key")

# supports cohere command-light with api_key
assert CohereInvocationLayer.supports("command-light", api_key="fake_key")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api_key is not used in supports method, why do we specify it here?

@vblagoje
Copy link
Member Author

vblagoje commented May 9, 2023

@bogdankostic I added 86d432a that we can then reuse for other layers, most notably for HF based layers, but some others potentially as well. In my experiments, I found gpt2 to be almost the same regarding token breakdown count. I used https://docs.cohere.com/reference/tokenize to count tokens in about five sample texts. The difference was within 1% to gpt2.

Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost good to go, just added two questions that are not clear to me + not sure about the added tests for PromptHandler.

Ensures CohereInvocationLayer is selected only when Cohere models are specified in
the model name.
"""
is_inference_api = "api_key" in kwargs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we check if the user has provided their api key? We don't do this in OpenAIInvocationLayer's `support method either, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an extra method of increasing the positive signal that we are using Cohere indeed. We might have some cases where the exact same model is in HuggingFace but HuggingFace doesn't have api_key passed. We can go without it here as well but it is just another check to make sure that we are indeed using cohere method. That's all

@@ -203,3 +203,20 @@ class HuggingFaceInferenceUnauthorizedError(HuggingFaceInferenceError):

def __init__(self, message: Optional[str] = None, send_message_in_event: bool = False):
super().__init__(message=message, status_code=401, send_message_in_event=send_message_in_event)


class CohereInferenceError(NodeError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between CohereInferenceError and CohereError?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None, good catch, let me update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the tests added to this file are unit tests, as they use transformers as external resource.

@vblagoje vblagoje marked this pull request as draft May 11, 2023 14:31
@vblagoje vblagoje marked this pull request as ready for review May 11, 2023 14:31
@github-actions github-actions bot removed the topic:CI label May 12, 2023
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@vblagoje vblagoje merged commit 73380b1 into main May 12, 2023
@vblagoje vblagoje deleted the cohere branch May 12, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Cohere instruction following model
3 participants