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(integrations): Add Anthropic Integration #2831

Merged
merged 13 commits into from May 2, 2024

Conversation

czyber
Copy link
Contributor

@czyber czyber commented Mar 16, 2024

This PR adds an anthropic integration. It supports the creation of messages in streaming and non-streaming mode.
It is heavily inspired/analogous to the OpenAI integration.

Docs PR: getsentry/sentry-docs#9469


General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@antonpirker
Copy link
Member

antonpirker commented Apr 25, 2024

Hey @czyber !

Thanks for this great PR!
As we released a big update to our SDK today, some of your code is broken!
I will allow myself to update your code to work with the new 2.0 version of the SDK.

This makes it then easier to review the integration and speeds things up so we can get this merged soon!

@antonpirker
Copy link
Member

Hey again @czyber ,

I made some changes to better deal with the spans. Everything looks great!

One question: Do the test actual send data to Anthropic? Or does the "z" api_key enable some sort of test mode? (because we do not want to access the internet in our tests, if it is not absolutely necessary)

@czyber
Copy link
Contributor Author

czyber commented Apr 25, 2024

Hey @antonpirker,

Thank you for updating the PR!

The tests do not send data to Anthropic, yet initializing the client with api_key="z" does not enable some kind of test mode, it does not send a request. Also, i mocked the part where requests would actually be sent (the _post attribute of SyncAPIResource)

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks again for the contribution!

I will hold on merging this for a bit, because we released 2.0 yesterday and maybe we need to do some hot fixes.

We also have a Langchain integration in the works right now, and I would like to release both in the same version, so we have a kind of AI update :-)

Copy link
Member

@colin-sentry colin-sentry left a comment

Choose a reason for hiding this comment

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

A few changes needed after langchain is merged, but this looks good otherwise. Nice work :)

if input_tokens != 0:
span.set_data(PROMPT_TOKENS_USED, input_tokens)
if output_tokens != 0:
span.set_data(COMPLETION_TOKENS_USED, output_tokens)
Copy link
Member

Choose a reason for hiding this comment

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


with capture_internal_exceptions():
span.set_data("ai.model_id", model)
span.set_data("ai.streaming", False)
Copy link
Member

Choose a reason for hiding this comment

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

"ai.responses",
[{"type": "text", "text": complete_message}],
)
span.set_data(TOTAL_TOKENS_USED, input_tokens + output_tokens)
Copy link
Member

Choose a reason for hiding this comment

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

New helper function again (after langchain merged)

assert "ai.input_messages" not in span["data"]
assert "ai.responses" not in span["data"]

assert span["data"][PROMPT_TOKENS_USED] == 10
Copy link
Member

Choose a reason for hiding this comment

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

Some of this has changed a bit in the langchain merge as well, it's span["measurements"] I believe

@czyber
Copy link
Contributor Author

czyber commented Apr 28, 2024

Hey @colin-sentry ,
thanks for the review!

I have the langchain PR on my mind, and will incorporate the requested changes as soon as it is merged 👍

@antonpirker
Copy link
Member

fyi @czyber the langchain branch has been merged into master!

czyber added 3 commits May 1, 2024 09:07
This commit updates the anthropic integration to include the newest changes from the langchain integration
@czyber
Copy link
Contributor Author

czyber commented May 1, 2024

fyi @colin-sentry @antonpirker , I updated the PR to use the shared functionality for AI integrations that was introduced in the langchain integration PR.

Copy link
Member

@colin-sentry colin-sentry left a comment

Choose a reason for hiding this comment

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

Looks good to me! Nice work.

@antonpirker
Copy link
Member

Yes! Great work everybody! Thanks a lot!

@antonpirker antonpirker merged commit eac253a into getsentry:master May 2, 2024
109 of 111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants