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 AnthropicGenerator and AnthropicChatGenerator #573

Merged
merged 8 commits into from
Mar 15, 2024

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Mar 12, 2024

Why:

This Pull Request introduces a new integration for the Haystack framework, enabling the use of Anthropic's Claude family of language models. Anthropic's API provides advanced features such as streaming responses and customizability, making it an excellent addition to Haystack's list of supported integrations.

What:

The PR adds:

  • A new top-level directory integrations/anthropic including:
    • src directory containing the integration implementation
    • tests directory with test files
    • pyproject.toml defining build and packaging information
    • pydoc directory with configuration and markdown file for automatically generated documentation
    • LICENSE.txt for the Apache License 2.0
  • An updated .github/workflows/anthropic.yml file to add CI workflows for testing and packaging
  • A new AnthropicChatGenerator and AnthropicGenerator for integrating with Anthropic's API
  • Necessary modifications to project's documentation in the docs directory
  • Test files and fixtures in the tests directory

How can it be used:

  • Use the AnthropicChatGenerator and AnthropicGenerator components in your pipelines to generate responses and text using Anthropic's Claude family of models
  • Utilize the streaming capabilities of these components to display responses in real-time

How did you test it:

  • Unit tests are conducted using pytest to validate the components' functionalities, such as their interaction with the Anthropic API, input and output handling, and the serialization and deserialization processes
  • Integration tests are performed to ensure compatibility with Haystack

Notes for the reviewer:

  • Ensure the API key is correctly passed to Anthropic's API and the required environment variable ANTHROPIC_API_KEY is set
  • Examine the ALLOWED_PARAMS variable in both generator classes to confirm that the allowed parameters for Anthropic's API are up-to-date
  • Validate that the integration with Haystack is correctly implemented and the components can be utilized with Haystack pipelines

@github-actions github-actions bot added the type:documentation Improvements or additions to documentation label Mar 12, 2024
@vblagoje vblagoje force-pushed the anthropic_integration branch from 9b79e07 to 122f56f Compare March 12, 2024 13:53
@vblagoje vblagoje marked this pull request as ready for review March 12, 2024 13:54
@vblagoje vblagoje requested a review from a team as a code owner March 12, 2024 13:54
@vblagoje vblagoje requested review from julian-risch and removed request for a team March 12, 2024 13:54
@vblagoje
Copy link
Member Author

@julian-risch not ready yet, just trying to run some CI, please don't review yet

@vblagoje vblagoje changed the title feat: Add AnthropicChatGenerator feat: Add AnthropicGenerator and AnthropicChatGenerator Mar 13, 2024
@julian-risch
Copy link
Member

We shouldn't use fixes https://github.com/deepset-ai/haystack-core-integrations/issues/139 in the PR description here. The issue contains more steps than what the PR covers.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks quite good. Very few small change requests. Not using inheritance is fine by me here.

.github/workflows/anthropic.yml Show resolved Hide resolved
integrations/anthropic/README.md Show resolved Hide resolved
markers = [
"unit: unit tests",
"integration: integration tests",
"generators: generators tests",
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a marker for chat generators
"chat_generators: chat generator tests",

Copy link
Member

Choose a reason for hiding this comment

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

And lets use the markers in the tests. Alternative would be to remove the "generators: generators tests", line here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove them, I don't see the point of using them :-)

"system",
"max_tokens",
"metadata",
# "stream", explicitly passed to the interface
Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove the line and if important mention it in docstrings.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 The markers can be helpful for running a subset of the tests more easily. Here we already achieve separation through the two test files.

@vblagoje vblagoje merged commit aca516c into main Mar 15, 2024
10 checks passed
@vblagoje vblagoje deleted the anthropic_integration branch March 15, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:CI type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AnthropicGenerator
2 participants